* [PATCH libmnl] nlmsg, attr: fix false positives when validating buffer sizes
@ 2023-09-10 20:30 Jeremy Sowden
2023-09-11 19:21 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Jeremy Sowden @ 2023-09-10 20:30 UTC (permalink / raw)
To: Netfilter Devel
`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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH libmnl] nlmsg, attr: fix false positives when validating buffer sizes
2023-09-10 20:30 [PATCH libmnl] nlmsg, attr: fix false positives when validating buffer sizes Jeremy Sowden
@ 2023-09-11 19:21 ` Pablo Neira Ayuso
2023-09-11 19:24 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2023-09-11 19:21 UTC (permalink / raw)
To: Jeremy Sowden; +Cc: Netfilter Devel
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH libmnl] nlmsg, attr: fix false positives when validating buffer sizes
2023-09-11 19:21 ` Pablo Neira Ayuso
@ 2023-09-11 19:24 ` Pablo Neira Ayuso
2023-09-11 20:30 ` Jeremy Sowden
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2023-09-11 19:24 UTC (permalink / raw)
To: Jeremy Sowden; +Cc: Netfilter Devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH libmnl] nlmsg, attr: fix false positives when validating buffer sizes
2023-09-11 19:24 ` Pablo Neira Ayuso
@ 2023-09-11 20:30 ` Jeremy Sowden
2023-09-11 21:17 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Jeremy Sowden @ 2023-09-11 20:30 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: Netfilter Devel
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH libmnl] nlmsg, attr: fix false positives when validating buffer sizes
2023-09-11 20:30 ` Jeremy Sowden
@ 2023-09-11 21:17 ` Pablo Neira Ayuso
2023-10-14 16:59 ` Jeremy Sowden
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2023-09-11 21:17 UTC (permalink / raw)
To: Jeremy Sowden; +Cc: Netfilter Devel
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 :)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH libmnl] nlmsg, attr: fix false positives when validating buffer sizes
2023-09-11 21:17 ` Pablo Neira Ayuso
@ 2023-10-14 16:59 ` Jeremy Sowden
0 siblings, 0 replies; 6+ messages in thread
From: Jeremy Sowden @ 2023-10-14 16:59 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: Netfilter Devel
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-10-14 16:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-10 20:30 [PATCH libmnl] nlmsg, attr: fix false positives when validating buffer sizes Jeremy Sowden
2023-09-11 19:21 ` Pablo Neira Ayuso
2023-09-11 19:24 ` Pablo Neira Ayuso
2023-09-11 20:30 ` Jeremy Sowden
2023-09-11 21:17 ` Pablo Neira Ayuso
2023-10-14 16:59 ` Jeremy Sowden
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).