[nft] mnl: catch bogus expressions before crashing

[nft] mnl: catch bogus expressions before crashing

[Florian Westphal]

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.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 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

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

[Pablo Neira Ayuso]

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
> 
>