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

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

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
>