Re: [PATCH libmnl] nlmsg, attr: fix false positives when validating buffer sizes

[Jeremy Sowden <jeremy@azazel.net>]

[-- Attachment #1: Type: text/plain, Size: 4627 bytes --]

On 2023-09-11, at 23:17:01 +0200, Pablo Neira Ayuso wrote:
> On Mon, Sep 11, 2023 at 09:30:26PM +0100, Jeremy Sowden wrote:
> > On 2023-09-11, at 21:24:05 +0200, Pablo Neira Ayuso wrote:
> > > On Mon, Sep 11, 2023 at 09:21:50PM +0200, Pablo Neira Ayuso wrote:
> > > > On Sun, Sep 10, 2023 at 09:30:18PM +0100, Jeremy Sowden wrote:
> > > > > `mnl_nlmsg_ok` and `mnl_attr_ok` both expect a signed buffer
> > > > > length value, `len`, against which to compare the size of the
> > > > > object expected to fit into the buffer, because they are intended
> > > > > to validate the length and it may be negative in the case of
> > > > > malformed messages.  Comparing this signed value against unsigned
> > > > > operands leads to compiler warnings, so the unsigned operands are
> > > > > cast to `int`.  Comparing `len` to the size of the structure is
> > > > > fine, because the structures are only a few bytes in size.
> > > > > Comparing it to the length fields of `struct nlmsg` and `struct
> > > > > nlattr`, however, is problematic, since these fields may hold
> > > > > values greater than `INT_MAX`, in which case the casts will yield
> > > > > negative values and result in false positives.
> > > > > 
> > > > > Instead, assign `len` to an unsigned local variable, check for
> > > > > negative values first, then use the unsigned local for the other
> > > > > comparisons, and remove the casts.
> > > > > 
> > > > > Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1691
> > > > > Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> > > > > ---
> > > > >  src/attr.c  | 9 +++++++--
> > > > >  src/nlmsg.c | 9 +++++++--
> > > > >  2 files changed, 14 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/src/attr.c b/src/attr.c
> > > > > index bc39df4199e7..48e95019d5e8 100644
> > > > > --- a/src/attr.c
> > > > > +++ b/src/attr.c
> > > > > @@ -97,9 +97,14 @@ EXPORT_SYMBOL void *mnl_attr_get_payload(const struct nlattr *attr)
> > > > >   */
> > > > >  EXPORT_SYMBOL bool mnl_attr_ok(const struct nlattr *attr, int len)
> > > > 
> > > > Maybe turn this into uint32_t ?
> > > 
> > > Actually, attribute length field is 16 bits long, so it can never
> > > happen that nla_len will underflow.
> > 
> > Oh, yeah.  Sorry.  I Thought I'd checked that.  I think my version
> > without the casts is still tidier.  My preference would be to keep the
> > nlattr change but amend the commit message, but if you prefer I'll drop
> > it.
> 
> I would prefer to update documentation and drop the change for nlattr.

No problem. I'll drop it.

> For mnl_attr_ok(), this should be sufficient?
> 
>         if (len < 0)
>                 return false;
> 
> to reject buffer larger that 2^31 (2 GBytes) or when length goes
> negative (malformed netlink message in the buffer).
> 
> BTW, this _trivial_ function is modeled after nlmsg_ok() in the
> kernel...
> 
> On Sun, Sep 10, 2023 at 09:30:18PM +0100, Jeremy Sowden wrote:
> > `mnl_nlmsg_ok` and `mnl_attr_ok` both expect a signed buffer length
> > value, `len`, against which to compare the size of the object expected
> > to fit into the buffer, because they are intended to validate the length       
> > and it may be negative in the case of malformed messages.  Comparing           
> > this signed value against unsigned operands leads to compiler warnings,        
> > so the unsigned operands are cast to `int`.
> 
> Did you see such compiler warning? If so, post it in commit
> description.
> 
> > Comparing `len` to the size of the structure is fine, because
> > the structures are only a few bytes in size. Comparing it to
> > the length fields of `struct nlmsg` and `struct nlattr`,
> > however, is problematic, since these fields may hold values greater
> > than `INT_MAX`, in which case the casts will yield negative values
> > and result in false positives.
> 
> Yes, but the early check for negative length prevents this after this
> patch, correct?

The problem arises if `len` is positive and `nlh->nlmsg_len` is larger
than INT_MAX.  In that case:

  len >= (int)sizeof(struct nlmsghdr)

succeeds because `len` is positive, and:

  (int)nlh->nlmsg_len <= len

succeeds because the cast yields a negative value, so we get a false
positive.

> > Instead, assign `len` to an unsigned local variable, check for negative        
> > values first, then use the unsigned local for the other comparisons, and       
> > remove the casts.
> 
> Makes sense.
> 
> Probably I'm getting confused with this large patch description :)

I'll trim it down. :)

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]