af_key: more info leaks in pfkey messages

PATCH Superseded Dan Carpenter 4 participants 8 messages Jul 24, 2013 – Jul 30, 2013

Subsystems (4)

Networking Network drivers Kernel janitors Networking IPsec

lore

Share & tools

lore

Dan Carpenter

author

2013-07-24 10:39 UTC #0 lore raw

PATCH 1/1 af_key: more info leaks in pfkey messages

This is inspired by a5cc68f3d6 "af_key: fix info leaks in notify messages". There are some struct members which don't get initialized and could disclose small amounts of private information.

Signed-off-by Dan Carpenter <[email protected]>

---
I'm at the "monkey see, monkey do" level of understanding this code so let me know if I've missed something.

net/key/af_key.c +4 -0

index 456b262..e939d32 100644
@@ -2081,6 +2081,7 @@ static int pfkey_xfrm_policy2msg(struct sk_buff *skb, const struct xfrm_policy *
 			pol->sadb_x_policy_type = IPSEC_POLICY_NONE;
 	}
 	pol->sadb_x_policy_dir = dir+1;
+	pol->sadb_x_policy_reserved = 0;
 	pol->sadb_x_policy_id = xp->index;
 	pol->sadb_x_policy_priority = xp->priority;

@@ -3137,7 +3138,9 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
 	pol->sadb_x_policy_exttype = SADB_X_EXT_POLICY;
 	pol->sadb_x_policy_type = IPSEC_POLICY_IPSEC;
 	pol->sadb_x_policy_dir = XFRM_POLICY_OUT + 1;
+	pol->sadb_x_policy_reserved = 0;
 	pol->sadb_x_policy_id = xp->index;
+	pol->sadb_x_policy_priority = 0;
 
 	/* Set sadb_comb's. */
 	if (x->id.proto = IPPROTO_AH)

@@ -3525,6 +3528,7 @@ static int pfkey_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 	pol->sadb_x_policy_exttype = SADB_X_EXT_POLICY;
 	pol->sadb_x_policy_type = IPSEC_POLICY_IPSEC;
 	pol->sadb_x_policy_dir = dir + 1;
+	pol->sadb_x_policy_reserved = 0;
 	pol->sadb_x_policy_id = 0;
 	pol->sadb_x_policy_priority = 0;

Mathias Krause

Acked-by on v1 1/1

2013-07-25 07:15 UTC #1 lore raw

Quoted 33 lines · Dan Carpenter

This is inspired by a5cc68f3d6 "af_key: fix info leaks in notify messages". There are some struct members which don't get initialized and could disclose small amounts of private information.

Signed-off-by Dan Carpenter <[email protected]>

---
I'm at the "monkey see, monkey do" level of understanding this code so let me know if I've missed something.

net/key/af_key.c +3 -0

index 456b262..e939d32 100644
@@ -2081,6 +2081,7 @@ static int pfkey_xfrm_policy2msg(struct sk_buff *skb, const struct xfrm_policy *
                        pol->sadb_x_policy_type = IPSEC_POLICY_NONE;
        }
        pol->sadb_x_policy_dir = dir+1;
+       pol->sadb_x_policy_reserved = 0;
        pol->sadb_x_policy_id = xp->index;
        pol->sadb_x_policy_priority = xp->priority;

@@ -3137,7 +3138,9 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
        pol->sadb_x_policy_exttype = SADB_X_EXT_POLICY;
        pol->sadb_x_policy_type = IPSEC_POLICY_IPSEC;
        pol->sadb_x_policy_dir = XFRM_POLICY_OUT + 1;
+       pol->sadb_x_policy_reserved = 0;
        pol->sadb_x_policy_id = xp->index;
+       pol->sadb_x_policy_priority = 0;
 
        /* Set sadb_comb's. */
        if (x->id.proto = IPPROTO_AH)

net/key/af_key.c static int pfkey_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type, · line 3534

Quoted 7 lines

net/key/af_key.c +1 -0

@@ -3525,6 +3528,7 @@ static int pfkey_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
        pol->sadb_x_policy_exttype = SADB_X_EXT_POLICY;
        pol->sadb_x_policy_type = IPSEC_POLICY_IPSEC;
        pol->sadb_x_policy_dir = dir + 1;
+       pol->sadb_x_policy_reserved = 0;
        pol->sadb_x_policy_id = 0;
        pol->sadb_x_policy_priority = 0;

Looks good to me. All end up being send to userland over the PF_KEY interface.

Acked-by Mathias Krause <[email protected]>

Steffen Klassert

maintainer

2013-07-25 07:32 UTC #2 lore raw

In reply to Dan Carpenter

Quoted 22 lines · Dan Carpenter

This is inspired by a5cc68f3d6 "af_key: fix info leaks in notify messages". There are some struct members which don't get initialized and could disclose small amounts of private information.
Signed-off-by Dan Carpenter <[email protected]>
---
I'm at the "monkey see, monkey do" level of understanding this code so let me know if I've missed something.
net/key/af_key.c +1 -0
index 456b262..e939d32 100644
@@ -2081,6 +2081,7 @@ static int pfkey_xfrm_policy2msg(struct sk_buff *skb, const struct xfrm_policy *
 			pol->sadb_x_policy_type = IPSEC_POLICY_NONE;
 	}
 	pol->sadb_x_policy_dir = dir+1;
+	pol->sadb_x_policy_reserved = 0;
 	pol->sadb_x_policy_id = xp->index;
 	pol->sadb_x_policy_priority = xp->priority;
 

net/key/af_key.c static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct · line 3143

Quoted 6 lines

net/key/af_key.c +2 -0

@@ -3137,7 +3138,9 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
 	pol->sadb_x_policy_exttype = SADB_X_EXT_POLICY;
 	pol->sadb_x_policy_type = IPSEC_POLICY_IPSEC;
 	pol->sadb_x_policy_dir = XFRM_POLICY_OUT + 1;
+	pol->sadb_x_policy_reserved = 0;
 	pol->sadb_x_policy_id = xp->index;
+	pol->sadb_x_policy_priority = 0;

Userspace seems not to care, but the correct setting would be

pol->sadb_x_policy_priority = xp->priority;

So maybe we should send the right priority to userspace, just for the case that anyone is interested in it.

Dan Carpenter

author

2013-07-25 08:24 UTC #3 lore raw

Quoted 7 lines · Steffen Klassert

Userspace seems not to care, but the correct setting would be
pol->sadb_x_policy_priority = xp->priority;
So maybe we should send the right priority to userspace, just for the case that anyone is interested in it.

Yep. Of course. Thanks. I will redo and resend.

regards,
dan carpenter

Dan Carpenter

author

2013-07-28 20:04 UTC #4 lore raw

PATCH v2 1/1 af_key: more info leaks in pfkey messages

In reply to Steffen Klassert

This is inspired by a5cc68f3d6 "af_key: fix info leaks in notify messages". There are some struct members which don't get initialized and could disclose small amounts of private information.

Acked-by Mathias Krause <[email protected]>

Signed-off-by Dan Carpenter <[email protected]>

---
v2: set ->sadb_x_policy_priority correctly instead of just clearing it

net/key/af_key.c +4 -0

index 456b262..d49f676 100644
@@ -2081,6 +2081,7 @@ static int pfkey_xfrm_policy2msg(struct sk_buff *skb, const struct xfrm_policy *
 			pol->sadb_x_policy_type = IPSEC_POLICY_NONE;
 	}
 	pol->sadb_x_policy_dir = dir+1;
+	pol->sadb_x_policy_reserved = 0;
 	pol->sadb_x_policy_id = xp->index;
 	pol->sadb_x_policy_priority = xp->priority;

@@ -3137,7 +3138,9 @@ static int pfkey_send_acquire(struct xfrm_state *x, struct xfrm_tmpl *t, struct
 	pol->sadb_x_policy_exttype = SADB_X_EXT_POLICY;
 	pol->sadb_x_policy_type = IPSEC_POLICY_IPSEC;
 	pol->sadb_x_policy_dir = XFRM_POLICY_OUT + 1;
+	pol->sadb_x_policy_reserved = 0;
 	pol->sadb_x_policy_id = xp->index;
+	pol->sadb_x_policy_priority = xp->priority;
 
 	/* Set sadb_comb's. */
 	if (x->id.proto = IPPROTO_AH)

@@ -3525,6 +3528,7 @@ static int pfkey_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 	pol->sadb_x_policy_exttype = SADB_X_EXT_POLICY;
 	pol->sadb_x_policy_type = IPSEC_POLICY_IPSEC;
 	pol->sadb_x_policy_dir = dir + 1;
+	pol->sadb_x_policy_reserved = 0;
 	pol->sadb_x_policy_id = 0;
 	pol->sadb_x_policy_priority = 0;

David Miller

maintainer

2013-07-28 20:21 UTC #5 lore raw

From: Dan Carpenter <[email protected]>
Date: Sun, 28 Jul 2013 23:04:45 +0300

Quoted 8 lines

This is inspired by a5cc68f3d6 "af_key: fix info leaks in notify messages". There are some struct members which don't get initialized and could disclose small amounts of private information.
Acked-by Mathias Krause <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
---
v2: set ->sadb_x_policy_priority correctly instead of just clearing it

I'll apply this as soon as Steffen reviews it.

Steffen Klassert

maintainer Acked-by

2013-07-29 09:16 UTC #6 lore raw

Quoted 7 lines · David Miller

From: Dan Carpenter <[email protected]>
Date: Sun, 28 Jul 2013 23:04:45 +0300
Quoted 4 lines
This is inspired by a5cc68f3d6 "af_key: fix info leaks in notify messages". There are some struct members which don't get initialized and could disclose small amounts of private information.

Quoted 6 lines

Quoted 4 lines
Acked-by Mathias Krause <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
---
v2: set ->sadb_x_policy_priority correctly instead of just clearing it
I'll apply this as soon as Steffen reviews it.

The patch looks good, for the case you want to apply it directly to the net tree:

Acked-by Steffen Klassert <[email protected]>

David Miller

maintainer

2013-07-30 23:26 UTC #7 lore raw

From: Steffen Klassert <[email protected]>
Date: Mon, 29 Jul 2013 11:16:53 +0200

Quoted 13 lines · David Miller

Quoted 12 lines
From: Dan Carpenter <[email protected]>
Date: Sun, 28 Jul 2013 23:04:45 +0300
Quoted 8 lines
This is inspired by a5cc68f3d6 "af_key: fix info leaks in notify messages". There are some struct members which don't get initialized and could disclose small amounts of private information.
Acked-by Mathias Krause <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
---
v2: set ->sadb_x_policy_priority correctly instead of just clearing it

Quoted 6 lines

Quoted 1 line
I'll apply this as soon as Steffen reviews it.
The patch looks good, for the case you want to apply it directly to the net tree:
Acked-by Steffen Klassert <[email protected]>

Applied and queued up for -stable, thanks everyone.