Re: [nft] mnl: catch bogus expressions before crashing

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

On Fri, Jun 06, 2025 at 12:20:28AM +0200, Florian Westphal wrote:
> We can't recover from errors here, but we can abort with a more
> precise reason than 'segmentation fault', or stack corruptions
> that get caught way later, or not at all.
> 
> expr->value is going to be read, we can't cope with other expression
> types here.
> 
> We will copy to stack buffer of IFNAMSIZ size, abort if we would
> overflow.
> 
> Check there is a NUL byte present too.
> This is a preemptive patch, I've seen one crash in this area but
> no reproducer yet.

I don't see how this can happen yet, but I like this sanity checks.

> Signed-off-by: Florian Westphal <fw@strlen.de>

Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org>

> ---
>  src/mnl.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/src/mnl.c b/src/mnl.c
> index 64b1aaedb84c..6565341fa6e3 100644
> --- a/src/mnl.c
> +++ b/src/mnl.c
> @@ -732,9 +732,20 @@ static void nft_dev_add(struct nft_dev *dev_array, const struct expr *expr, int
>  	unsigned int ifname_len;
>  	char ifname[IFNAMSIZ];
>  
> +	if (expr->etype != EXPR_VALUE)
> +		BUG("Must be a value, not %s\n", expr_name(expr));
> +
>  	ifname_len = div_round_up(expr->len, BITS_PER_BYTE);
>  	memset(ifname, 0, sizeof(ifname));
> +
> +	if (ifname_len > sizeof(ifname))
> +		BUG("Interface length %u exceeds limit\n", ifname_len);
> +
>  	mpz_export_data(ifname, expr->value, BYTEORDER_HOST_ENDIAN, ifname_len);
> +
> +	if (strnlen(ifname, IFNAMSIZ) >= IFNAMSIZ)
> +		BUG("Interface length %zu exceeds limit, no NUL byte\n", strnlen(ifname, IFNAMSIZ));
> +
>  	dev_array[i].ifname = xstrdup(ifname);
>  	dev_array[i].location = &expr->location;
>  }
> -- 
> 2.49.0
> 
>