[5/6] xfrm_user: ensure user supplied esn replay window is valid
diff mbox series

Message ID 1348090423-32665-6-git-send-email-minipli@googlemail.com
State New, archived
Headers show
Series
  • xfrm_user info leaks
Related show

Commit Message

Mathias Krause Sept. 19, 2012, 9:33 p.m. UTC
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(-)

Comments

Ben Hutchings Sept. 19, 2012, 10:38 p.m. UTC | #1
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.
Mathias Krause Sept. 20, 2012, 6:12 a.m. UTC | #2
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.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Steffen Klassert Sept. 20, 2012, 7:05 a.m. UTC | #3
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mathias Krause Sept. 20, 2012, 7:13 a.m. UTC | #4
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mathias Krause Sept. 20, 2012, 7:37 a.m. UTC | #5
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch
diff mbox series

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;