| Message ID | 1348090423-32665-6-git-send-email-minipli@googlemail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)