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

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

[Florian Westphal]

The bogon will trigger the assertion in mpz_import_data:
src/expression.c:418: constant_expr_alloc: Assertion `(((len) + (8) - 1) / (8)) > 0' failed.

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

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

[Pablo Neira Ayuso]

[-- 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));
 	}

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

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 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.

        return constant_expr_alloc(int_loc, &string_type, BYTEORDER_HOST_ENDIAN,
                                   strlen(chain) * BITS_PER_BYTE, chain);

chain name is '""'.

There are other spots where we possibly call into constant_expr_alloc()
with a 0 argument.

I think it would be a lot more work and bloat to add all the checks on
the json side while its a one-liner in constant_expr_alloc().

I could also add json_constant_expr_alloc() but it seems kinda silly to me.

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

[Pablo Neira Ayuso]

On Mon, Mar 31, 2025 at 02:37:15PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > 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.
>
>         return constant_expr_alloc(int_loc, &string_type, BYTEORDER_HOST_ENDIAN,
>                                    strlen(chain) * BITS_PER_BYTE, chain);
>
> chain name is '""'.
>
> There are other spots where we possibly call into constant_expr_alloc()
> with a 0 argument.

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

> I think it would be a lot more work and bloat to add all the checks on
> the json side while its a one-liner in constant_expr_alloc().

I don't think this is needed, the idiom in src/parser_json.c is to:

1) fetch string
2) validate it

For example:

        if (!json_unpack(root, "{s:s}", "ttl", &ttl)) {
                if (!strcmp(ttl, "loose")) {
                        ttlval = 1;
                } else if (!strcmp(ttl, "skip")) {
                        ttlval = 2;
                } else {
                        json_error(ctx, "Invalid osf ttl option '%s'.", ttl);
                        return NULL;
                }
        }

> I could also add json_constant_expr_alloc() but it seems kinda silly to me.

I agree, I don't think json_constant_expr_alloc is needed.