lkml.org 
[lkml]   [2012]   [Sep]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 5/6] xfrm_user: ensure user supplied esn replay window is valid
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.



\
 
 \ /
  Last update: 2012-09-20 09:41    [W:0.077 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site