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

[Jeremy Sowden <jeremy@azazel.net>]

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

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.

J.

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