Re: [PATCH nft] expression: don't try to import empty string

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

[-- Attachment #1: Type: text/plain, Size: 2419 bytes --]

Hi Florian,

On Thu, Mar 27, 2025 at 04:17:11PM +0100, Florian Westphal wrote:
> The bogon will trigger the assertion in mpz_import_data:
> src/expression.c:418: constant_expr_alloc: Assertion `(((len) + (8) - 1) / (8)) > 0' failed.

I took a quick look searching for {s:s} in src/parser_json.c

The common idiom is json_parse_err() then a helper parser function to
validate the string.

It seems it is missing in this case. Maybe tigthen json parser instead?

Caller invoking constant_expr_alloc() with data != NULL but no len
looks broken to me.

Maybe take both patches if you prefer?

> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  src/expression.c                              |  2 +-
>  .../bogons/nft-j-f/constant_expr_alloc_assert | 38 +++++++++++++++++++
>  2 files changed, 39 insertions(+), 1 deletion(-)
>  create mode 100644 tests/shell/testcases/bogons/nft-j-f/constant_expr_alloc_assert
> 
> diff --git a/src/expression.c b/src/expression.c
> index 156a66eb37f0..f230f5ad8935 100644
> --- a/src/expression.c
> +++ b/src/expression.c
> @@ -494,7 +494,7 @@ struct expr *constant_expr_alloc(const struct location *loc,
>  	expr->flags = EXPR_F_CONSTANT | EXPR_F_SINGLETON;
>  
>  	mpz_init2(expr->value, len);
> -	if (data != NULL)
> +	if (data != NULL && len)
>  		mpz_import_data(expr->value, data, byteorder,
>  				div_round_up(len, BITS_PER_BYTE));
>  
> diff --git a/tests/shell/testcases/bogons/nft-j-f/constant_expr_alloc_assert b/tests/shell/testcases/bogons/nft-j-f/constant_expr_alloc_assert
> new file mode 100644
> index 000000000000..9c40030212ef
> --- /dev/null
> +++ b/tests/shell/testcases/bogons/nft-j-f/constant_expr_alloc_assert
> @@ -0,0 +1,38 @@
> +{
> +  "nftables": [
> +    {
> +      "table": {
> +        "family": "ip",
> +        "name": "t",
> +        "handle": 0
> +      }
> +    },
> +    {
> +      "chain": {
> +        "family": "ip",
> +        "table": "t",
> +        "name": "testchain",
> +        "handle": 0
> +      }
> +    },
> +    {
> +      "map": {
> +        "family": "ip",
> +        "name": "testmap",
> +        "table": "t",
> +        "type": "ipv4_addr",
> +        "handle": 0,
> +        "map": "verdict",
> +        "elem": [
> +          [
> +            {
> +              "jump": {
> +                "target": ""
> +              }
> +            }
> +          ]
> +        ]
> +      }
> +    }
> +  ]
> +}
> -- 
> 2.48.1
> 
> 

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 467 bytes --]

diff --git a/src/parser_json.c b/src/parser_json.c
index 04d762741e4a..ef7740840710 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -1350,6 +1350,9 @@ static struct expr *json_parse_verdict_expr(struct json_ctx *ctx,
 		    json_unpack_err(ctx, root, "{s:s}", "target", &chain))
 			return NULL;
 
+		if (!chain || chain[0] == '\0')
+			return NULL;
+
 		return verdict_expr_alloc(int_loc, verdict_tbl[i].verdict,
 					  json_alloc_chain_expr(chain));
 	}