From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752694Ab2ISWjO (ORCPT ); Wed, 19 Sep 2012 18:39:14 -0400 Received: from webmail.solarflare.com ([12.187.104.25]:53231 "EHLO ocex02.SolarFlarecom.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752627Ab2ISWid (ORCPT ); Wed, 19 Sep 2012 18:38:33 -0400 Message-ID: <1348094309.2636.80.camel@bwh-desktop.uk.solarflarecom.com> Subject: Re: [PATCH 5/6] xfrm_user: ensure user supplied esn replay window is valid From: Ben Hutchings To: Mathias Krause CC: "David S. Miller" , Steffen Klassert , , , Martin Willi Date: Wed, 19 Sep 2012 23:38:29 +0100 In-Reply-To: <1348090423-32665-6-git-send-email-minipli@googlemail.com> References: <1348090423-32665-1-git-send-email-minipli@googlemail.com> <1348090423-32665-6-git-send-email-minipli@googlemail.com> Organization: Solarflare Communications Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3 (3.2.3-3.fc16) Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-Originating-IP: [10.17.20.137] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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.