[PATCH nft 1/2] evaluate: compact STMT_F_STATEFUL checks

[PATCH nft 1/2] evaluate: compact STMT_F_STATEFUL checks

[Florian Westphal]

We'll gain another F_STATEFUL check in a followup patch,
so lets condense the pattern into a helper to reduce copypaste.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/evaluate.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index e4a7b5ceaafa..e9ab829b6bbb 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3453,6 +3453,17 @@ static int stmt_evaluate_payload(struct eval_ctx *ctx, struct stmt *stmt)
 	return expr_evaluate(ctx, &stmt->payload.val);
 }
 
+static int stmt_evaluate_stateful(struct eval_ctx *ctx, struct stmt *stmt, const char *name)
+{
+	if (stmt_evaluate(ctx, stmt) < 0)
+		return -1;
+
+	if (!(stmt->flags & STMT_F_STATEFUL))
+		return stmt_error(ctx, stmt, "%s statement must be stateful", name);
+
+	return 0;
+}
+
 static int stmt_evaluate_meter(struct eval_ctx *ctx, struct stmt *stmt)
 {
 	struct expr *key, *setref;
@@ -3526,11 +3537,8 @@ static int stmt_evaluate_meter(struct eval_ctx *ctx, struct stmt *stmt)
 
 	stmt->meter.set = setref;
 
-	if (stmt_evaluate(ctx, stmt->meter.stmt) < 0)
+	if (stmt_evaluate_stateful(ctx, stmt->meter.stmt, "meter") < 0)
 		return -1;
-	if (!(stmt->meter.stmt->flags & STMT_F_STATEFUL))
-		return stmt_binary_error(ctx, stmt->meter.stmt, stmt,
-					 "meter statement must be stateful");
 
 	return 0;
 }
@@ -4662,11 +4670,8 @@ static int stmt_evaluate_set(struct eval_ctx *ctx, struct stmt *stmt)
 		return expr_error(ctx->msgs, stmt->set.key,
 				  "Key expression comments are not supported");
 	list_for_each_entry(this, &stmt->set.stmt_list, list) {
-		if (stmt_evaluate(ctx, this) < 0)
+		if (stmt_evaluate_stateful(ctx, this, "set") < 0)
 			return -1;
-		if (!(this->flags & STMT_F_STATEFUL))
-			return stmt_error(ctx, this,
-					  "statement must be stateful");
 	}
 
 	this_set = stmt->set.set->set;
@@ -4726,11 +4731,8 @@ static int stmt_evaluate_map(struct eval_ctx *ctx, struct stmt *stmt)
 				  "Data expression timeouts are not supported");
 
 	list_for_each_entry(this, &stmt->map.stmt_list, list) {
-		if (stmt_evaluate(ctx, this) < 0)
+		if (stmt_evaluate_stateful(ctx, this, "map") < 0)
 			return -1;
-		if (!(this->flags & STMT_F_STATEFUL))
-			return stmt_error(ctx, this,
-					  "statement must be stateful");
 	}
 
 	return 0;
-- 
2.49.0

[PATCH nft 2/2] evaluate: only allow stateful statements in set and map definitions

[Florian Westphal]

The bison parser doesn't allow this to happen due to grammar
restrictions, but the json input has no such issues.

The bogon input assigns 'notrack' which triggers:
BUG: unknown stateful statement type 19
nft: src/netlink_linearize.c:1061: netlink_gen_stmt_stateful: Assertion `0' failed.

After patch, we get:
Error: map statement must be stateful

Fixes: 07958ec53830 ("json: add set statement list support")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/evaluate.c                                |  5 ++-
 .../unkown_stateful_statement_type_19_assert  | 34 +++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 tests/shell/testcases/bogons/nft-j-f/unkown_stateful_statement_type_19_assert

diff --git a/src/evaluate.c b/src/evaluate.c
index e9ab829b6bbb..f73edc916406 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -5157,8 +5157,11 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
 	if (set->timeout)
 		set->flags |= NFT_SET_TIMEOUT;
 
-	list_for_each_entry(stmt, &set->stmt_list, list)
+	list_for_each_entry(stmt, &set->stmt_list, list) {
+		if (stmt_evaluate_stateful(ctx, stmt,type) < 0)
+			return -1;
 		num_stmts++;
+	}
 
 	if (num_stmts > 1)
 		set->flags |= NFT_SET_EXPR;
diff --git a/tests/shell/testcases/bogons/nft-j-f/unkown_stateful_statement_type_19_assert b/tests/shell/testcases/bogons/nft-j-f/unkown_stateful_statement_type_19_assert
new file mode 100644
index 000000000000..e8a0f768d754
--- /dev/null
+++ b/tests/shell/testcases/bogons/nft-j-f/unkown_stateful_statement_type_19_assert
@@ -0,0 +1,34 @@
+{
+  "nftables": [
+    {
+      "metainfo": {
+        "version": "VERSION",
+        "release_name": "RELEASE_NAME",
+        "json_schema_version": 1
+      }
+    },
+    {
+      "table": {
+        "family": "ip",
+        "name": "t",
+        "handle": 0
+      }
+    },
+    {
+      "map": {
+        "family": "ip",
+        "name": "m",
+        "table": "t",
+        "type": "ipv4_addr",
+        "handle": 0,
+        "map": "mark",
+        "stmt": [
+          {
+            "notrack": null
+          }
+        ]
+      }
+    }
+  ]
+}
+
-- 
2.49.0

Re: [PATCH nft 2/2] evaluate: only allow stateful statements in set and map definitions

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> The bison parser doesn't allow this to happen due to grammar
> restrictions, but the json input has no such issues.
> 
> The bogon input assigns 'notrack' which triggers:
> BUG: unknown stateful statement type 19
> nft: src/netlink_linearize.c:1061: netlink_gen_stmt_stateful: Assertion `0' failed.
> 
> After patch, we get:
> Error: map statement must be stateful

On the same subject of 'do I fix this in evaluate.c or parser_json.c':

cat bla
table t {
        set sc {
                type inet_service . ifname
        }

        chain c {
                tcp dport . bla* @sc accept
        }
}
nft -f bla
BUG: unknown expression type prefix
nft: src/netlink_linearize.c:914: netlink_gen_expr: Assertion `0' failed.
Aborted (core dumped)

I can either fix this in evaluate.c, or I try to rework both
parser_bison.y and parser_json.c to no longer allow prefix expressions
when specifying the lookup key.

I suspect that fixing it in evaluate.c is going to be a lot simpler.

We can't disable prefixes in concatenations, its valid for set element
keys, but I suspect that we can use recursion counter to figure out if
the concatenation is on the RHS of something else (such as part of
EXPR_SET_ELEM).

I'll work on it tomorrow.

Re: [PATCH nft 2/2] evaluate: only allow stateful statements in set and map definitions

[Pablo Neira Ayuso]

On Mon, Mar 31, 2025 at 06:15:30PM +0200, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > The bison parser doesn't allow this to happen due to grammar
> > restrictions, but the json input has no such issues.
> > 
> > The bogon input assigns 'notrack' which triggers:
> > BUG: unknown stateful statement type 19
> > nft: src/netlink_linearize.c:1061: netlink_gen_stmt_stateful: Assertion `0' failed.
> > 
> > After patch, we get:
> > Error: map statement must be stateful
> 
> On the same subject of 'do I fix this in evaluate.c or parser_json.c':
> 
> cat bla
> table t {
>         set sc {
>                 type inet_service . ifname
>         }
> 
>         chain c {
>                 tcp dport . bla* @sc accept
>         }
> }
> nft -f bla
> BUG: unknown expression type prefix
> nft: src/netlink_linearize.c:914: netlink_gen_expr: Assertion `0' failed.
> Aborted (core dumped)
> 
> I can either fix this in evaluate.c, or I try to rework both
> parser_bison.y and parser_json.c to no longer allow prefix expressions
> when specifying the lookup key.

It is probably too complex from the parser in this case.

> I suspect that fixing it in evaluate.c is going to be a lot simpler.

I agree.

But for things that are obviously incorrect at parsing stage and that
are simple to reject, like the empty string case for goto/jump in
verdict map, then it is easy to reject early. The json parser already
rejects unexisting/unsupported keys, the parser is already making a
first pass in validating the input.

I don't think it is a matter of picking between validating _only_ from
parser or from evaluation, I think tightening from parser (when
trivial) and evaluation for more complex invalid input is fine.

> We can't disable prefixes in concatenations, its valid for set element
> keys, but I suspect that we can use recursion counter to figure out if
> the concatenation is on the RHS of something else (such as part of
> EXPR_SET_ELEM).
> 
> I'll work on it tomorrow.

Thanks!

Re: [PATCH nft 1/2] evaluate: compact STMT_F_STATEFUL checks

[Pablo Neira Ayuso]

On Mon, Mar 31, 2025 at 05:23:19PM +0200, Florian Westphal wrote:
> We'll gain another F_STATEFUL check in a followup patch,
> so lets condense the pattern into a helper to reduce copypaste.

This series LGTM, thanks Florian.

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

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

> ---
>  src/evaluate.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index e4a7b5ceaafa..e9ab829b6bbb 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -3453,6 +3453,17 @@ static int stmt_evaluate_payload(struct eval_ctx *ctx, struct stmt *stmt)
>  	return expr_evaluate(ctx, &stmt->payload.val);
>  }
>  
> +static int stmt_evaluate_stateful(struct eval_ctx *ctx, struct stmt *stmt, const char *name)
> +{
> +	if (stmt_evaluate(ctx, stmt) < 0)
> +		return -1;
> +
> +	if (!(stmt->flags & STMT_F_STATEFUL))
> +		return stmt_error(ctx, stmt, "%s statement must be stateful", name);
> +
> +	return 0;
> +}
> +
>  static int stmt_evaluate_meter(struct eval_ctx *ctx, struct stmt *stmt)
>  {
>  	struct expr *key, *setref;
> @@ -3526,11 +3537,8 @@ static int stmt_evaluate_meter(struct eval_ctx *ctx, struct stmt *stmt)
>  
>  	stmt->meter.set = setref;
>  
> -	if (stmt_evaluate(ctx, stmt->meter.stmt) < 0)
> +	if (stmt_evaluate_stateful(ctx, stmt->meter.stmt, "meter") < 0)
>  		return -1;
> -	if (!(stmt->meter.stmt->flags & STMT_F_STATEFUL))
> -		return stmt_binary_error(ctx, stmt->meter.stmt, stmt,
> -					 "meter statement must be stateful");
>  
>  	return 0;
>  }
> @@ -4662,11 +4670,8 @@ static int stmt_evaluate_set(struct eval_ctx *ctx, struct stmt *stmt)
>  		return expr_error(ctx->msgs, stmt->set.key,
>  				  "Key expression comments are not supported");
>  	list_for_each_entry(this, &stmt->set.stmt_list, list) {
> -		if (stmt_evaluate(ctx, this) < 0)
> +		if (stmt_evaluate_stateful(ctx, this, "set") < 0)
>  			return -1;
> -		if (!(this->flags & STMT_F_STATEFUL))
> -			return stmt_error(ctx, this,
> -					  "statement must be stateful");
>  	}
>  
>  	this_set = stmt->set.set->set;
> @@ -4726,11 +4731,8 @@ static int stmt_evaluate_map(struct eval_ctx *ctx, struct stmt *stmt)
>  				  "Data expression timeouts are not supported");
>  
>  	list_for_each_entry(this, &stmt->map.stmt_list, list) {
> -		if (stmt_evaluate(ctx, this) < 0)
> +		if (stmt_evaluate_stateful(ctx, this, "map") < 0)
>  			return -1;
> -		if (!(this->flags & STMT_F_STATEFUL))
> -			return stmt_error(ctx, this,
> -					  "statement must be stateful");
>  	}
>  
>  	return 0;
> -- 
> 2.49.0
> 
>