lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 19 Sep 2012 23:38:29 +0100 From: Ben Hutchings <bhutchings@...arflare.com> To: Mathias Krause <minipli@...glemail.com> CC: "David S. Miller" <davem@...emloft.net>, Steffen Klassert <steffen.klassert@...unet.com>, <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Martin Willi <martin@...osec.ch> Subject: Re: [PATCH 5/6] xfrm_user: ensure user supplied esn replay window is valid 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists