diff mbox

[-mainline] af_key: more info leaks in pfkey messages

Message ID 20130724103956.GA30345@elgon.mountain
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Dan Carpenter July 24, 2013, 10:39 a.m. UTC
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 <dan.carpenter@oracle.com>
---
I'm at the "monkey see, monkey do" level of understanding this code so
let me know if I've missed something.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Mathias Krause July 25, 2013, 7:15 a.m. UTC | #1
On 24 July 2013 12:39, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> 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 <dan.carpenter@oracle.com>
> ---
> I'm at the "monkey see, monkey do" level of understanding this code so
> let me know if I've missed something.
>
> diff --git a/net/key/af_key.c b/net/key/af_key.c
> index 456b262..e939d32 100644
> --- a/net/key/af_key.c
> +++ b/net/key/af_key.c
> @@ -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;
>

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

Acked-by: Mathias Krause <minipli@googlemail.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steffen Klassert July 25, 2013, 7:32 a.m. UTC | #2
On Wed, Jul 24, 2013 at 01:39:57PM +0300, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
> ---
> I'm at the "monkey see, monkey do" level of understanding this code so
> let me know if I've missed something.
> 
> diff --git a/net/key/af_key.c b/net/key/af_key.c
> index 456b262..e939d32 100644
> --- a/net/key/af_key.c
> +++ b/net/key/af_key.c
> @@ -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;

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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Carpenter July 25, 2013, 8:24 a.m. UTC | #3
On Thu, Jul 25, 2013 at 09:32:06AM +0200, Steffen Klassert wrote:
> 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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/key/af_key.c b/net/key/af_key.c
index 456b262..e939d32 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -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;