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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
