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

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

[Jeremy Sowden]

`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)
 {
-	return len >= (int)sizeof(struct nlattr) &&
+	size_t ulen = len;
+
+	if (len < 0)
+		return 0;
+
+	return ulen          >= sizeof(struct nlattr) &&
 	       attr->nla_len >= sizeof(struct nlattr) &&
-	       (int)attr->nla_len <= len;
+	       attr->nla_len <= ulen;
 }
 
 /**
diff --git a/src/nlmsg.c b/src/nlmsg.c
index c63450174c67..920cad0e0f46 100644
--- a/src/nlmsg.c
+++ b/src/nlmsg.c
@@ -152,9 +152,14 @@ EXPORT_SYMBOL void *mnl_nlmsg_get_payload_offset(const struct nlmsghdr *nlh,
  */
 EXPORT_SYMBOL bool mnl_nlmsg_ok(const struct nlmsghdr *nlh, int len)
 {
-	return len >= (int)sizeof(struct nlmsghdr) &&
+	size_t ulen = len;
+
+	if (len < 0)
+		return 0;
+
+	return ulen           >= sizeof(struct nlmsghdr) &&
 	       nlh->nlmsg_len >= sizeof(struct nlmsghdr) &&
-	       (int)nlh->nlmsg_len <= len;
+	       nlh->nlmsg_len <= ulen;
 }
 
 /**
-- 
2.40.1

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

[Pablo Neira Ayuso]

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 ?

>  {
> -	return len >= (int)sizeof(struct nlattr) &&
> +	size_t ulen = len;
> +
> +	if (len < 0)
> +		return 0;
> +
> +	return ulen          >= sizeof(struct nlattr) &&
>  	       attr->nla_len >= sizeof(struct nlattr) &&
> -	       (int)attr->nla_len <= len;
> +	       attr->nla_len <= ulen;
>  }
>  
>  /**
> diff --git a/src/nlmsg.c b/src/nlmsg.c
> index c63450174c67..920cad0e0f46 100644
> --- a/src/nlmsg.c
> +++ b/src/nlmsg.c
> @@ -152,9 +152,14 @@ EXPORT_SYMBOL void *mnl_nlmsg_get_payload_offset(const struct nlmsghdr *nlh,
>   */
>  EXPORT_SYMBOL bool mnl_nlmsg_ok(const struct nlmsghdr *nlh, int len)
>  {
> -	return len >= (int)sizeof(struct nlmsghdr) &&
> +	size_t ulen = len;
> +
> +	if (len < 0)
> +		return 0;
> +
> +	return ulen           >= sizeof(struct nlmsghdr) &&
>  	       nlh->nlmsg_len >= sizeof(struct nlmsghdr) &&
> -	       (int)nlh->nlmsg_len <= len;
> +	       nlh->nlmsg_len <= ulen;
>  }
>  
>  /**
> -- 
> 2.40.1
> 

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

[Pablo Neira Ayuso]

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.

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

[Jeremy Sowden]

[-- 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 --]

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

[Pablo Neira Ayuso]

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.

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?

> 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 :)

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

[Jeremy Sowden]

[-- 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 --]