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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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.