| Message ID | 20230910203018.2782009-1-jeremy@azazel.net |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [libmnl] nlmsg, attr: fix false positives when validating buffer sizes | expand |
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 >
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.
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.
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 :)
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.
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; } /**
`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(-)