[PATCH 0/6] xfrm_user info leaks

[PATCH 0/6] xfrm_user info leaks

[Mathias Krause]

Hi David,

the following series fixes various info leaks in the xfrm netlink
interface. As always, a test case can be supplied on request.

Patches 1 to 5 are probably material for stable, too. Patch 6 is just a
minor optimization I stumbled across while auditing the code.

Please apply!

P.S.: What's still missing IMHO, is some upper limit for the ESN replay
window size to prevent malicious users from making the kernel allocate
huge states. One could not retrieve those via netlink anyway (as the
current code allocates just a single page for the state dump). Maybe 4k
is a sane default upper limit for the replay window size. But I'll leave
implementing this to someone else. ;)


Mathias Krause (6):
  xfrm_user: fix info leak in copy_to_user_auth()
  xfrm_user: fix info leak in copy_to_user_state()
  xfrm_user: fix info leak in copy_to_user_policy()
  xfrm_user: fix info leak in copy_to_user_tmpl()
  xfrm_user: ensure user supplied esn replay window is valid
  xfrm_user: don't copy esn replay window twice for new states

 net/xfrm/xfrm_user.c |   41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

-- 
1.7.10.4

[PATCH 1/6] xfrm_user: fix info leak in copy_to_user_auth()

[Mathias Krause]

copy_to_user_auth() fails to initialize the remainder of alg_name and
therefore discloses up to 54 bytes of heap memory via netlink to
userland.

Use strncpy() instead of strcpy() to fill the trailing bytes of alg_name
with null bytes.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 net/xfrm/xfrm_user.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index e75d8e4..ba26423 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -742,7 +742,7 @@ static int copy_to_user_auth(struct xfrm_algo_auth *auth, struct sk_buff *skb)
 		return -EMSGSIZE;
 
 	algo = nla_data(nla);
-	strcpy(algo->alg_name, auth->alg_name);
+	strncpy(algo->alg_name, auth->alg_name, sizeof(algo->alg_name));
 	memcpy(algo->alg_key, auth->alg_key, (auth->alg_key_len + 7) / 8);
 	algo->alg_key_len = auth->alg_key_len;
 
-- 
1.7.10.4

[PATCH 2/6] xfrm_user: fix info leak in copy_to_user_state()

[Mathias Krause]

The memory reserved to dump the xfrm state includes the padding bytes of
struct xfrm_usersa_info added by the compiler for alignment (7 for
amd64, 3 for i386). Add an explicit memset(0) before filling the buffer
to avoid the info leak.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 net/xfrm/xfrm_user.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index ba26423..c1f15b5 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -689,6 +689,7 @@ out:
 
 static void copy_to_user_state(struct xfrm_state *x, struct xfrm_usersa_info *p)
 {
+	memset(p, 0, sizeof(*p));
 	memcpy(&p->id, &x->id, sizeof(p->id));
 	memcpy(&p->sel, &x->sel, sizeof(p->sel));
 	memcpy(&p->lft, &x->lft, sizeof(p->lft));
-- 
1.7.10.4

[PATCH 3/6] xfrm_user: fix info leak in copy_to_user_policy()

[Mathias Krause]

The memory reserved to dump the xfrm policy includes multiple padding
bytes added by the compiler for alignment (padding bytes in struct
xfrm_selector and struct xfrm_userpolicy_info). Add an explicit
memset(0) before filling the buffer to avoid the heap info leak.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 net/xfrm/xfrm_user.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index c1f15b5..7511427 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1318,6 +1318,7 @@ static void copy_from_user_policy(struct xfrm_policy *xp, struct xfrm_userpolicy
 
 static void copy_to_user_policy(struct xfrm_policy *xp, struct xfrm_userpolicy_info *p, int dir)
 {
+	memset(p, 0, sizeof(*p));
 	memcpy(&p->sel, &xp->selector, sizeof(p->sel));
 	memcpy(&p->lft, &xp->lft, sizeof(p->lft));
 	memcpy(&p->curlft, &xp->curlft, sizeof(p->curlft));
-- 
1.7.10.4

[PATCH 4/6] xfrm_user: fix info leak in copy_to_user_tmpl()

[Mathias Krause]

The memory used for the template copy is a local stack variable. As
struct xfrm_user_tmpl contains multiple holes added by the compiler for
alignment, not initializing the memory will lead to leaking stack bytes
to userland. Add an explicit memset(0) to avoid the info leak.

Initial version of the patch by Brad Spengler.

Cc: Brad Spengler <spender@grsecurity.net>
Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 net/xfrm/xfrm_user.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 7511427..9f1e749 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1423,6 +1423,7 @@ static int copy_to_user_tmpl(struct xfrm_policy *xp, struct sk_buff *skb)
 		struct xfrm_user_tmpl *up = &vec[i];
 		struct xfrm_tmpl *kp = &xp->xfrm_vec[i];
 
+		memset(up, 0, sizeof(*up));
 		memcpy(&up->id, &kp->id, sizeof(up->id));
 		up->family = kp->encap_family;
 		memcpy(&up->saddr, &kp->saddr, sizeof(up->saddr));
-- 
1.7.10.4

Re: [PATCH 4/6] xfrm_user: fix info leak in copy_to_user_tmpl()

[Steffen Klassert]

On Wed, Sep 19, 2012 at 11:33:41PM +0200, Mathias Krause wrote:
> The memory used for the template copy is a local stack variable. As
> struct xfrm_user_tmpl contains multiple holes added by the compiler for
> alignment, not initializing the memory will lead to leaking stack bytes
> to userland. Add an explicit memset(0) to avoid the info leak.
> 
> Initial version of the patch by Brad Spengler.
> 
> Cc: Brad Spengler <spender@grsecurity.net>
> Signed-off-by: Mathias Krause <minipli@googlemail.com>

Patches 1-4:

Acked-by: Steffen Klassert <steffen.klassert@secunet.com>

[PATCH 5/6] xfrm_user: ensure user supplied esn replay window is valid

[Mathias Krause]

The current code fails to ensure that the netlink message actually
contains as many bytes as the header indicates. If a user creates a new
state or updates an existing one but does not supply the bytes for the
whole ESN replay window, the kernel copies random heap bytes into the
replay bitmap, the ones happen to follow the XFRMA_REPLAY_ESN_VAL
netlink attribute. This leads to following issues:

1. The replay window has random bits set confusing the replay handling
   code later on.

2. A malicious user could use this flaw to leak up to ~3.5kB of heap
   memory when she has access to the XFRM netlink interface (requires
   CAP_NET_ADMIN).

Known users of the ESN replay window are strongSwan and Steffen's
iproute2 patch (<http://patchwork.ozlabs.org/patch/85962/>). The latter
uses the interface with a bitmap supplied while the former does not.
strongSwan is therefore prone to run into issue 1.

To fix both issues without breaking existing userland allow using the
XFRMA_REPLAY_ESN_VAL netlink attribute with either an empty bitmap or a
fully specified one. For the former case we initialize the in-kernel
bitmap with zero, for the latter we copy the user supplied bitmap.

For state updates the full bitmap must be supplied.

Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Martin Willi <martin@revosec.ch>
Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 net/xfrm/xfrm_user.c |   27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 9f1e749..7fd92b8 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -123,9 +123,17 @@ static inline int verify_replay(struct xfrm_usersa_info *p,
 				struct nlattr **attrs)
 {
 	struct nlattr *rt = attrs[XFRMA_REPLAY_ESN_VAL];
+	struct xfrm_replay_state_esn *rs;
 
-	if ((p->flags & XFRM_STATE_ESN) && !rt)
-		return -EINVAL;
+	if (p->flags & XFRM_STATE_ESN) {
+		if (!rt)
+			return -EINVAL;
+
+		rs = nla_data(rt);
+		if (nla_len(rt) < xfrm_replay_state_esn_len(rs) &&
+		    nla_len(rt) != sizeof(*rs))
+			return -EINVAL;
+	}
 
 	if (!rt)
 		return 0;
@@ -370,14 +378,15 @@ static inline int xfrm_replay_verify_len(struct xfrm_replay_state_esn *replay_es
 					 struct nlattr *rp)
 {
 	struct xfrm_replay_state_esn *up;
+	size_t ulen;
 
 	if (!replay_esn || !rp)
 		return 0;
 
 	up = nla_data(rp);
+	ulen = xfrm_replay_state_esn_len(up);
 
-	if (xfrm_replay_state_esn_len(replay_esn) !=
-			xfrm_replay_state_esn_len(up))
+	if (nla_len(rp) < ulen || xfrm_replay_state_esn_len(replay_esn) != ulen)
 		return -EINVAL;
 
 	return 0;
@@ -388,22 +397,28 @@ static int xfrm_alloc_replay_state_esn(struct xfrm_replay_state_esn **replay_esn
 				       struct nlattr *rta)
 {
 	struct xfrm_replay_state_esn *p, *pp, *up;
+	size_t klen, ulen;
 
 	if (!rta)
 		return 0;
 
 	up = nla_data(rta);
+	klen = xfrm_replay_state_esn_len(up);
+	ulen = nla_len(rta) > sizeof(*up) ? klen : sizeof(*up);
 
-	p = kmemdup(up, xfrm_replay_state_esn_len(up), GFP_KERNEL);
+	p = kzalloc(klen, GFP_KERNEL);
 	if (!p)
 		return -ENOMEM;
 
-	pp = kmemdup(up, xfrm_replay_state_esn_len(up), GFP_KERNEL);
+	pp = kzalloc(klen, GFP_KERNEL);
 	if (!pp) {
 		kfree(p);
 		return -ENOMEM;
 	}
 
+	memcpy(p, up, ulen);
+	memcpy(pp, up, ulen);
+
 	*replay_esn = p;
 	*preplay_esn = pp;
 
-- 
1.7.10.4

Re: [PATCH 5/6] xfrm_user: ensure user supplied esn replay window is valid

[Ben Hutchings]

On Wed, 2012-09-19 at 23:33 +0200, Mathias Krause wrote:
> The current code fails to ensure that the netlink message actually
> contains as many bytes as the header indicates. If a user creates a new
> state or updates an existing one but does not supply the bytes for the
> whole ESN replay window, the kernel copies random heap bytes into the
> replay bitmap, the ones happen to follow the XFRMA_REPLAY_ESN_VAL
> netlink attribute. This leads to following issues:
> 
> 1. The replay window has random bits set confusing the replay handling
>    code later on.
> 
> 2. A malicious user could use this flaw to leak up to ~3.5kB of heap
>    memory when she has access to the XFRM netlink interface (requires
>    CAP_NET_ADMIN).

Where does this limit come from?  Is that just the standard size netlink
skb?

I'm a little worried that the user-provided
xfrm_replay_state_esn::bmp_len is not being directly validated anywhere.
Currently xfrm_replay_state_esn_len() may overflow, and as its return
type is int it may unexpectedly return a negative value.

[...]
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
[...]
> @@ -370,14 +378,15 @@ static inline int xfrm_replay_verify_len(struct xfrm_replay_state_esn *replay_es
>  					 struct nlattr *rp)
>  {
>  	struct xfrm_replay_state_esn *up;
> +	size_t ulen;

I would normally expect to see sizes declared as size_t but mixing
size_t and int in comparisons tends to result in bugs.  So I think this
should to be int, matching the return types of nla_len() and
xfrm_replay_state_esn_len() (and apparently all lengths in netlink...)

>  	if (!replay_esn || !rp)
>  		return 0;
>  
>  	up = nla_data(rp);
> +	ulen = xfrm_replay_state_esn_len(up);
>
> -	if (xfrm_replay_state_esn_len(replay_esn) !=
> -			xfrm_replay_state_esn_len(up))
> +	if (nla_len(rp) < ulen || xfrm_replay_state_esn_len(replay_esn) != ulen)
>  		return -EINVAL;
>  
>  	return 0;
> @@ -388,22 +397,28 @@ static int xfrm_alloc_replay_state_esn(struct xfrm_replay_state_esn **replay_esn
>  				       struct nlattr *rta)
>  {
>  	struct xfrm_replay_state_esn *p, *pp, *up;
> +	size_t klen, ulen;

Also int, for the same reason.

>  	if (!rta)
>  		return 0;
>  
>  	up = nla_data(rta);
> +	klen = xfrm_replay_state_esn_len(up);
> +	ulen = nla_len(rta) > sizeof(*up) ? klen : sizeof(*up);
[...]

I understand that this is correct since verify_replay() previously
checked that nla_len(rta) is either == sizeof(*up) or >= klen.  But
would it not be more obviously correct to test nla_len(rta) >= klen?

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 5/6] xfrm_user: ensure user supplied esn replay window is valid

[Mathias Krause]

On Thu, Sep 20, 2012 at 12:38 AM, Ben Hutchings
<bhutchings@solarflare.com> wrote:
> On Wed, 2012-09-19 at 23:33 +0200, Mathias Krause wrote:
>> The current code fails to ensure that the netlink message actually
>> contains as many bytes as the header indicates. If a user creates a new
>> state or updates an existing one but does not supply the bytes for the
>> whole ESN replay window, the kernel copies random heap bytes into the
>> replay bitmap, the ones happen to follow the XFRMA_REPLAY_ESN_VAL
>> netlink attribute. This leads to following issues:
>>
>> 1. The replay window has random bits set confusing the replay handling
>>    code later on.
>>
>> 2. A malicious user could use this flaw to leak up to ~3.5kB of heap
>>    memory when she has access to the XFRM netlink interface (requires
>>    CAP_NET_ADMIN).
>
> Where does this limit come from?  Is that just the standard size netlink
> skb?

It's from then "msg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC)" in
xfrm_state_netlink() which boils down to roughly 3.7k free space for
the netlink message. Excluding the space used for struct
xfrm_usersa_info and some minimal extensions leaves roughly 3.5k for
the replay window.
Maybe that's just another bug and the code should allocate a netlink
message big enough for the whole state dump. Don't know. I'm not
familiar with the code.

> I'm a little worried that the user-provided
> xfrm_replay_state_esn::bmp_len is not being directly validated anywhere.

That's what my P.S. in the cover letter tried to hint at -- a missing
upper limit check. But as I wanted to avoid lengthy discussions about
the concrete value and the possible need for some sysctl knob to tune
this even further, I just left this as an exercise for someone else
who is more familiar with the code ;)

> Currently xfrm_replay_state_esn_len() may overflow, and as its return
> type is int it may unexpectedly return a negative value.

So xfrm_replay_state_esn_len() should return size_t instead as it's
value should always be positive -- it represents a length. Negative
lengths make no sense. It can overflow, still. But it cannot get
negative, at least. Still, the upper limit check would be required to
avoid other user induced nastiness.

>
> [...]
>> --- a/net/xfrm/xfrm_user.c
>> +++ b/net/xfrm/xfrm_user.c
> [...]
>> @@ -370,14 +378,15 @@ static inline int xfrm_replay_verify_len(struct xfrm_replay_state_esn *replay_es
>>                                        struct nlattr *rp)
>>  {
>>       struct xfrm_replay_state_esn *up;
>> +     size_t ulen;
>
> I would normally expect to see sizes declared as size_t but mixing
> size_t and int in comparisons tends to result in bugs.  So I think this
> should to be int, matching the return types of nla_len() and
> xfrm_replay_state_esn_len() (and apparently all lengths in netlink...)

I disagree. The value of nla_len() is ensured to be in the range of
[sizeof(*up), USHRT_MAX-NLA_HDRLEN], i.e. a positive 16 bit number,
when it passes nlmsg_parse() in xfrm_user_rcv_msg(). This in turn
allows us to assume the int value returned by nla_len() is actually
positive and the compiler can safely make it unsigned for the compare
-- no sign bit, no hassle.
What still might happen is the overflow in xfrm_replay_state_esn_len()
resulting in a to small bitmap allocation for the requested replay
size. But that gets catched in xfrm_init_replay(). Little late, but
hey.

>
>>       if (!replay_esn || !rp)
>>               return 0;
>>
>>       up = nla_data(rp);
>> +     ulen = xfrm_replay_state_esn_len(up);
>>
>> -     if (xfrm_replay_state_esn_len(replay_esn) !=
>> -                     xfrm_replay_state_esn_len(up))
>> +     if (nla_len(rp) < ulen || xfrm_replay_state_esn_len(replay_esn) != ulen)
>>               return -EINVAL;
>>
>>       return 0;
>> @@ -388,22 +397,28 @@ static int xfrm_alloc_replay_state_esn(struct xfrm_replay_state_esn **replay_esn
>>                                      struct nlattr *rta)
>>  {
>>       struct xfrm_replay_state_esn *p, *pp, *up;
>> +     size_t klen, ulen;
>
> Also int, for the same reason.

No, for the reason stated above. I'll fixup
xfrm_replay_state_esn_len() to return size_t instead.

>
>>       if (!rta)
>>               return 0;
>>
>>       up = nla_data(rta);
>> +     klen = xfrm_replay_state_esn_len(up);
>> +     ulen = nla_len(rta) > sizeof(*up) ? klen : sizeof(*up);
> [...]
>
> I understand that this is correct since verify_replay() previously
> checked that nla_len(rta) is either == sizeof(*up) or >= klen.  But
> would it not be more obviously correct to test nla_len(rta) >= klen?

It is. Comparing against klen makes the code more readable.

Thanks,
Mathias

>
> Ben.
>
> --
> Ben Hutchings, Staff Engineer, Solarflare
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>

[PATCH v2] xfrm_user: ensure user supplied esn replay window is valid

[Mathias Krause]

The current code fails to ensure that the netlink message actually
contains as many bytes as the header indicates. If a user creates a new
state or updates an existing one but does not supply the bytes for the
whole ESN replay window, the kernel copies random heap bytes into the
replay bitmap, the ones happen to follow the XFRMA_REPLAY_ESN_VAL
netlink attribute. This leads to following issues:

1. The replay window has random bits set confusing the replay handling
   code later on.

2. A malicious user could use this flaw to leak up to ~3.5kB of heap
   memory when she has access to the XFRM netlink interface (requires
   CAP_NET_ADMIN).

Known users of the ESN replay window are strongSwan and Steffen's
iproute2 patch (<http://patchwork.ozlabs.org/patch/85962/>). The latter
uses the interface with a bitmap supplied while the former does not.
strongSwan is therefore prone to run into issue 1.

To fix both issues without breaking existing userland allow using the
XFRMA_REPLAY_ESN_VAL netlink attribute with either an empty bitmap or a
fully specified one. For the former case we initialize the in-kernel
bitmap with zero, for the latter we copy the user supplied bitmap.

For state updates the full bitmap must be supplied.

While at it, fix xfrm_replay_state_esn_len() to return size_t instead of
int as it calculates a length and all users expect the return value to
be positive.

Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Martin Willi <martin@revosec.ch>
Cc: Ben Hutchings <bhutchings@solarflare.com>
Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
v2:
- compare against klen in xfrm_alloc_replay_state_esn (suggested by Ben)
- make xfrm_replay_state_esn_len() return size_t

 include/net/xfrm.h   |    4 ++--
 net/xfrm/xfrm_user.c |   27 +++++++++++++++++++++------
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 639dd13..3f7eadd 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1621,9 +1621,9 @@ static inline int xfrm_alg_auth_len(const struct xfrm_algo_auth *alg)
 	return sizeof(*alg) + ((alg->alg_key_len + 7) / 8);
 }
 
-static inline int xfrm_replay_state_esn_len(struct xfrm_replay_state_esn *replay_esn)
+static inline size_t xfrm_replay_state_esn_len(struct xfrm_replay_state_esn *rs)
 {
-	return sizeof(*replay_esn) + replay_esn->bmp_len * sizeof(__u32);
+	return sizeof(*rs) + rs->bmp_len * sizeof(__u32);
 }
 
 #ifdef CONFIG_XFRM_MIGRATE
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 9f1e749..44c4b98 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -123,9 +123,17 @@ static inline int verify_replay(struct xfrm_usersa_info *p,
 				struct nlattr **attrs)
 {
 	struct nlattr *rt = attrs[XFRMA_REPLAY_ESN_VAL];
+	struct xfrm_replay_state_esn *rs;
 
-	if ((p->flags & XFRM_STATE_ESN) && !rt)
-		return -EINVAL;
+	if (p->flags & XFRM_STATE_ESN) {
+		if (!rt)
+			return -EINVAL;
+
+		rs = nla_data(rt);
+		if (nla_len(rt) < xfrm_replay_state_esn_len(rs) &&
+		    nla_len(rt) != sizeof(*rs))
+			return -EINVAL;
+	}
 
 	if (!rt)
 		return 0;
@@ -370,14 +378,15 @@ static inline int xfrm_replay_verify_len(struct xfrm_replay_state_esn *replay_es
 					 struct nlattr *rp)
 {
 	struct xfrm_replay_state_esn *up;
+	size_t ulen;
 
 	if (!replay_esn || !rp)
 		return 0;
 
 	up = nla_data(rp);
+	ulen = xfrm_replay_state_esn_len(up);
 
-	if (xfrm_replay_state_esn_len(replay_esn) !=
-			xfrm_replay_state_esn_len(up))
+	if (nla_len(rp) < ulen || xfrm_replay_state_esn_len(replay_esn) != ulen)
 		return -EINVAL;
 
 	return 0;
@@ -388,22 +397,28 @@ static int xfrm_alloc_replay_state_esn(struct xfrm_replay_state_esn **replay_esn
 				       struct nlattr *rta)
 {
 	struct xfrm_replay_state_esn *p, *pp, *up;
+	size_t klen, ulen;
 
 	if (!rta)
 		return 0;
 
 	up = nla_data(rta);
+	klen = xfrm_replay_state_esn_len(up);
+	ulen = nla_len(rta) >= klen ? klen : sizeof(*up);
 
-	p = kmemdup(up, xfrm_replay_state_esn_len(up), GFP_KERNEL);
+	p = kzalloc(klen, GFP_KERNEL);
 	if (!p)
 		return -ENOMEM;
 
-	pp = kmemdup(up, xfrm_replay_state_esn_len(up), GFP_KERNEL);
+	pp = kzalloc(klen, GFP_KERNEL);
 	if (!pp) {
 		kfree(p);
 		return -ENOMEM;
 	}
 
+	memcpy(p, up, ulen);
+	memcpy(pp, up, ulen);
+
 	*replay_esn = p;
 	*preplay_esn = pp;
 
-- 
1.7.10.4

Re: [PATCH 5/6] xfrm_user: ensure user supplied esn replay window is valid

[Steffen Klassert]

On Thu, Sep 20, 2012 at 08:12:11AM +0200, Mathias Krause wrote:
> On Thu, Sep 20, 2012 at 12:38 AM, Ben Hutchings
> <bhutchings@solarflare.com> wrote:
> > On Wed, 2012-09-19 at 23:33 +0200, Mathias Krause wrote:
> 
> > I'm a little worried that the user-provided
> > xfrm_replay_state_esn::bmp_len is not being directly validated anywhere.
> 
> That's what my P.S. in the cover letter tried to hint at -- a missing
> upper limit check. But as I wanted to avoid lengthy discussions about
> the concrete value and the possible need for some sysctl knob to tune
> this even further, I just left this as an exercise for someone else
> who is more familiar with the code ;)
> 

I think we should limit bmp_len to some sane value. RFC 4303 recommends
an anti replay window size of 64 packets, so limiting bmp_len to cover
4096 packets should be more that enough. Also we can increase this value
later without changing the user API if this is needed.

> > Currently xfrm_replay_state_esn_len() may overflow, and as its return
> > type is int it may unexpectedly return a negative value.
> 
> So xfrm_replay_state_esn_len() should return size_t instead as it's
> value should always be positive -- it represents a length. Negative
> lengths make no sense. It can overflow, still. But it cannot get
> negative, at least. Still, the upper limit check would be required to
> avoid other user induced nastiness.
> 
> >
> > [...]
> >> --- a/net/xfrm/xfrm_user.c
> >> +++ b/net/xfrm/xfrm_user.c
> > [...]
> >> @@ -370,14 +378,15 @@ static inline int xfrm_replay_verify_len(struct xfrm_replay_state_esn *replay_es
> >>                                        struct nlattr *rp)
> >>  {
> >>       struct xfrm_replay_state_esn *up;
> >> +     size_t ulen;
> >
> > I would normally expect to see sizes declared as size_t but mixing
> > size_t and int in comparisons tends to result in bugs.  So I think this
> > should to be int, matching the return types of nla_len() and
> > xfrm_replay_state_esn_len() (and apparently all lengths in netlink...)
> 
> I disagree. The value of nla_len() is ensured to be in the range of
> [sizeof(*up), USHRT_MAX-NLA_HDRLEN], i.e. a positive 16 bit number,
> when it passes nlmsg_parse() in xfrm_user_rcv_msg(). This in turn
> allows us to assume the int value returned by nla_len() is actually
> positive and the compiler can safely make it unsigned for the compare
> -- no sign bit, no hassle.

I think xfrm_replay_state_esn_len() should return the same type as
nla_len(), no matter what we can assume from the current code base.
Also it should not return anything else than the other xfrm length
calculation functions.

Once we limited bmp_len, xfrm_replay_state_esn_len() should return
always a positive value.

Re: [PATCH 5/6] xfrm_user: ensure user supplied esn replay window is valid

[Mathias Krause]

On Thu, Sep 20, 2012 at 9:05 AM, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> On Thu, Sep 20, 2012 at 08:12:11AM +0200, Mathias Krause wrote:
>> On Thu, Sep 20, 2012 at 12:38 AM, Ben Hutchings
>> <bhutchings@solarflare.com> wrote:
>> > On Wed, 2012-09-19 at 23:33 +0200, Mathias Krause wrote:
>>
>> > I'm a little worried that the user-provided
>> > xfrm_replay_state_esn::bmp_len is not being directly validated anywhere.
>>
>> That's what my P.S. in the cover letter tried to hint at -- a missing
>> upper limit check. But as I wanted to avoid lengthy discussions about
>> the concrete value and the possible need for some sysctl knob to tune
>> this even further, I just left this as an exercise for someone else
>> who is more familiar with the code ;)
>>
>
> I think we should limit bmp_len to some sane value. RFC 4303 recommends
> an anti replay window size of 64 packets, so limiting bmp_len to cover
> 4096 packets should be more that enough. Also we can increase this value
> later without changing the user API if this is needed.

Okay. If no-one objects, I'll at add a upper limit check for 4096
packets to verify_replay().

>> [...]
>> I disagree. The value of nla_len() is ensured to be in the range of
>> [sizeof(*up), USHRT_MAX-NLA_HDRLEN], i.e. a positive 16 bit number,
>> when it passes nlmsg_parse() in xfrm_user_rcv_msg(). This in turn
>> allows us to assume the int value returned by nla_len() is actually
>> positive and the compiler can safely make it unsigned for the compare
>> -- no sign bit, no hassle.
>
> I think xfrm_replay_state_esn_len() should return the same type as
> nla_len(), no matter what we can assume from the current code base.

The type of the expression calculated in xfrm_replay_state_esn_len()
is size_t; the functions the value get passed onto (k*alloc, kmemdup,
memcpy, memcmp) expect a size_t argument; expressions where the value
is evaluated to calculate sizes (e.g. in xfrm_sa_len) operate on
size_t types. So size_t feels just natural.

> Also it should not return anything else than the other xfrm length
> calculation functions.

So the other functions should have a return type of size_t, too?

Anyway, such a cleanup should go into a separate patch as the other
functions are not vulnerable to an overflow like it could happen in
xfrm_replay_state_esn_len().

> Once we limited bmp_len, xfrm_replay_state_esn_len() should return
> always a positive value.

True. So int it'll be then again for xfrm_replay_state_esn_len() in v3
of the patch.


Thanks,
Mathias

[PATCH v3 5/7] xfrm_user: ensure user supplied esn replay window is valid

[Mathias Krause]

The current code fails to ensure that the netlink message actually
contains as many bytes as the header indicates. If a user creates a new
state or updates an existing one but does not supply the bytes for the
whole ESN replay window, the kernel copies random heap bytes into the
replay bitmap, the ones happen to follow the XFRMA_REPLAY_ESN_VAL
netlink attribute. This leads to following issues:

1. The replay window has random bits set confusing the replay handling
   code later on.

2. A malicious user could use this flaw to leak up to ~3.5kB of heap
   memory when she has access to the XFRM netlink interface (requires
   CAP_NET_ADMIN).

Known users of the ESN replay window are strongSwan and Steffen's
iproute2 patch (<http://patchwork.ozlabs.org/patch/85962/>). The latter
uses the interface with a bitmap supplied while the former does not.
strongSwan is therefore prone to run into issue 1.

To fix both issues without breaking existing userland allow using the
XFRMA_REPLAY_ESN_VAL netlink attribute with either an empty bitmap or a
fully specified one. For the former case we initialize the in-kernel
bitmap with zero, for the latter we copy the user supplied bitmap. For
state updates the full bitmap must be supplied.

To prevent overflows in the bitmap length calculation the maximum size
of bmp_len is limited to 128 by this patch -- resulting in a maximum
replay window of 4096 packets. This should be sufficient for all real
life scenarios (RFC 4303 recommends a default replay window size of 64).

Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Martin Willi <martin@revosec.ch>
Cc: Ben Hutchings <bhutchings@solarflare.com>
Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
v3:
- revert size_t change to xfrm_replay_state_esn_len() (requested by Steffen)
- switch to int types for lengths (suggested by Ben)
- implement 4096 packets limit for bmp_len to avoid overflows in
  xfrm_replay_state_esn_len()
v2:
- compare against klen in xfrm_alloc_replay_state_esn (suggested by Ben)
- make xfrm_replay_state_esn_len() return size_t

 include/linux/xfrm.h |    2 ++
 net/xfrm/xfrm_user.c |   31 +++++++++++++++++++++++++------
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/include/linux/xfrm.h b/include/linux/xfrm.h
index 22e61fd..28e493b 100644
--- a/include/linux/xfrm.h
+++ b/include/linux/xfrm.h
@@ -84,6 +84,8 @@ struct xfrm_replay_state {
 	__u32	bitmap;
 };
 
+#define XFRMA_REPLAY_ESN_MAX	4096
+
 struct xfrm_replay_state_esn {
 	unsigned int	bmp_len;
 	__u32		oseq;
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 9f1e749..e761562 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -123,9 +123,21 @@ static inline int verify_replay(struct xfrm_usersa_info *p,
 				struct nlattr **attrs)
 {
 	struct nlattr *rt = attrs[XFRMA_REPLAY_ESN_VAL];
+	struct xfrm_replay_state_esn *rs;
 
-	if ((p->flags & XFRM_STATE_ESN) && !rt)
-		return -EINVAL;
+	if (p->flags & XFRM_STATE_ESN) {
+		if (!rt)
+			return -EINVAL;
+
+		rs = nla_data(rt);
+
+		if (rs->bmp_len > XFRMA_REPLAY_ESN_MAX / sizeof(rs->bmp[0]) / 8)
+			return -EINVAL;
+
+		if (nla_len(rt) < xfrm_replay_state_esn_len(rs) &&
+		    nla_len(rt) != sizeof(*rs))
+			return -EINVAL;
+	}
 
 	if (!rt)
 		return 0;
@@ -370,14 +382,15 @@ static inline int xfrm_replay_verify_len(struct xfrm_replay_state_esn *replay_es
 					 struct nlattr *rp)
 {
 	struct xfrm_replay_state_esn *up;
+	int ulen;
 
 	if (!replay_esn || !rp)
 		return 0;
 
 	up = nla_data(rp);
+	ulen = xfrm_replay_state_esn_len(up);
 
-	if (xfrm_replay_state_esn_len(replay_esn) !=
-			xfrm_replay_state_esn_len(up))
+	if (nla_len(rp) < ulen || xfrm_replay_state_esn_len(replay_esn) != ulen)
 		return -EINVAL;
 
 	return 0;
@@ -388,22 +401,28 @@ static int xfrm_alloc_replay_state_esn(struct xfrm_replay_state_esn **replay_esn
 				       struct nlattr *rta)
 {
 	struct xfrm_replay_state_esn *p, *pp, *up;
+	int klen, ulen;
 
 	if (!rta)
 		return 0;
 
 	up = nla_data(rta);
+	klen = xfrm_replay_state_esn_len(up);
+	ulen = nla_len(rta) >= klen ? klen : sizeof(*up);
 
-	p = kmemdup(up, xfrm_replay_state_esn_len(up), GFP_KERNEL);
+	p = kzalloc(klen, GFP_KERNEL);
 	if (!p)
 		return -ENOMEM;
 
-	pp = kmemdup(up, xfrm_replay_state_esn_len(up), GFP_KERNEL);
+	pp = kzalloc(klen, GFP_KERNEL);
 	if (!pp) {
 		kfree(p);
 		return -ENOMEM;
 	}
 
+	memcpy(p, up, ulen);
+	memcpy(pp, up, ulen);
+
 	*replay_esn = p;
 	*preplay_esn = pp;
 
-- 
1.7.10.4

Re: [PATCH 5/6] xfrm_user: ensure user supplied esn replay window is valid

[Mathias Krause]

On Thu, Sep 20, 2012 at 8:12 AM, Mathias Krause <minipli@googlemail.com> wrote:
> What still might happen is the overflow in xfrm_replay_state_esn_len()
> resulting in a to small bitmap allocation for the requested replay
> size. But that gets catched in xfrm_init_replay(). Little late, but
> hey.

Sorry, I mixed that up. The replay_window check in xfrm_init_replay()
has only little to do with the bmp_len overflow. But changing the
return type of xfrm_replay_state_esn_len() to size_t and by doing so,
making the all the size compares operating on positive values, we'll
at least allocate enough memory to not run into memory corruptions.
Though, the replay window will be much smaller, than requested -- due
to the overflow. But userland should expect this. A check for some
upper limit in verify_replay() could catch this early.

Mathias

[PATCH 6/6] xfrm_user: don't copy esn replay window twice for new states

[Mathias Krause]

The ESN replay window was already fully initialized in
xfrm_alloc_replay_state_esn(). No need to copy it again.

Cc: Steffen Klassert <steffen.klassert@secunet.com>
Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 net/xfrm/xfrm_user.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 7fd92b8..fa072de 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -457,10 +457,11 @@ static void copy_from_user_state(struct xfrm_state *x, struct xfrm_usersa_info *
  * somehow made shareable and move it to xfrm_state.c - JHS
  *
 */
-static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs)
+static void xfrm_update_ae_params(struct xfrm_state *x, struct nlattr **attrs,
+				  int update_esn)
 {
 	struct nlattr *rp = attrs[XFRMA_REPLAY_VAL];
-	struct nlattr *re = attrs[XFRMA_REPLAY_ESN_VAL];
+	struct nlattr *re = update_esn ? attrs[XFRMA_REPLAY_ESN_VAL] : NULL;
 	struct nlattr *lt = attrs[XFRMA_LTIME_VAL];
 	struct nlattr *et = attrs[XFRMA_ETIMER_THRESH];
 	struct nlattr *rt = attrs[XFRMA_REPLAY_THRESH];
@@ -570,7 +571,7 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
 		goto error;
 
 	/* override default values from above */
-	xfrm_update_ae_params(x, attrs);
+	xfrm_update_ae_params(x, attrs, 0);
 
 	return x;
 
@@ -1840,7 +1841,7 @@ static int xfrm_new_ae(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto out;
 
 	spin_lock_bh(&x->lock);
-	xfrm_update_ae_params(x, attrs);
+	xfrm_update_ae_params(x, attrs, 1);
 	spin_unlock_bh(&x->lock);
 
 	c.event = nlh->nlmsg_type;
-- 
1.7.10.4

Re: [PATCH 6/6] xfrm_user: don't copy esn replay window twice for new states

[Steffen Klassert]

On Wed, Sep 19, 2012 at 11:33:43PM +0200, Mathias Krause wrote:
> The ESN replay window was already fully initialized in
> xfrm_alloc_replay_state_esn(). No need to copy it again.
> 
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Signed-off-by: Mathias Krause <minipli@googlemail.com>

Acked-by: Steffen Klassert <steffen.klassert@secunet.com>

Re: [PATCH 0/6] xfrm_user info leaks

[David Miller]

From: Mathias Krause <minipli@googlemail.com>
Date: Wed, 19 Sep 2012 23:33:37 +0200

> the following series fixes various info leaks in the xfrm netlink
> interface. As always, a test case can be supplied on request.
> 
> Patches 1 to 5 are probably material for stable, too. Patch 6 is just a
> minor optimization I stumbled across while auditing the code.
> 
> Please apply!

All applied, and I made sure to use v3 of patch #5 (which you marked
as 5/7 instead of 5/6 :-)

Also, these have been queued up for -stable as well.

Thanks.

Re: [PATCH 0/6] xfrm_user info leaks

[Mathias Krause]

On Fri, Sep 21, 2012 at 12:09 AM, David Miller <davem@davemloft.net> wrote:
> From: Mathias Krause <minipli@googlemail.com>
> Date: Wed, 19 Sep 2012 23:33:37 +0200
>
>> the following series fixes various info leaks in the xfrm netlink
>> interface. As always, a test case can be supplied on request.
>>
>> Patches 1 to 5 are probably material for stable, too. Patch 6 is just a
>> minor optimization I stumbled across while auditing the code.
>>
>> Please apply!
>
> All applied, and I made sure to use v3 of patch #5 (which you marked
> as 5/7 instead of 5/6 :-)

Sorry for the confusion. Looks like I've to learn a few more git
tricks, so this won't happen again ;)

> Also, these have been queued up for -stable as well.

Thanks!