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@xxxxxxxxxx> > > > > > --- > > > > > 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:
signature.asc
Description: PGP signature
