[PATCH nft 0/4] assorted fixes

[PATCH nft 0/4] assorted fixes

[Pablo Neira Ayuso]

Hi Florian,

This in an alternative path to address the issue described here:

https://patchwork.ozlabs.org/project/netfilter-devel/patch/20240110082657.1967-2-fw@strlen.de/

(I am partially integrating this patch into 3/4 in this series).

Patch #1 fixes a bug in the set optimization with single elements, which
         results in strange ruleset listings. Concatenations are only
	 supported with sets, so let's just skip this.

Patch #2 do not fetch next key in case runaway flag is set on with
	 concatenations. I could not crash nftables with this, but
	 I found it when reviewing this code.

Patch #3 iterate over the anonymous set in set_evaluate() to validate
         consistency of elements as concat expressions. Otherwise, bail
	 out with:

  ruleset.nft:3:46-53: Error: expression is not a concatenation
               ip protocol . th dport vmap { tcp / 22 : accept, tcp . 80 : drop}
                                             ^^^^^^^^

         I extended tests to cover maps too.

I needed special error handling when set_evaluate() fails to release sets so
ASAN does not complain with incorrect memory handling, this chunk:

@@ -118,7 +119,15 @@ static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
                list_add_tail(&cmd->list, &ctx->cmd->list);
        }

-       set_evaluate(ctx, set);
+       err = set_evaluate(ctx, set);
+       if (err < 0) {
+               list_del(&cmd->list);
+               if (set->flags & NFT_SET_MAP)
+                       cmd->set->init = NULL;
+
+               cmd_free(cmd);
+               return NULL;
+       }

        return set_ref_expr_alloc(&expr->location, set);
 }

Patch #4 revert a recent late sanity check which is already covered by this
	 patchset.

In general the idea is to make stricter validations from evaluation phase
to avoid propagating errors any further.

This passing tests/shell and tests/py, it might be a good idea to add more
bogons tests for concatenations with imbalanced number of components / runaway
number of components, I will follow up with a patch to extend tests
infrastructure with this.

Pablo Neira Ayuso (4):
  evaluate: skip anonymous set optimization for concatenations
  evaluate: do not fetch next expression on runaway number of concatenation components
  evaluate: bail out if anonymous concat set defines a non concat expression
  Revert "datatype: do not assert when value exceeds expected width"

 src/datatype.c                                |  6 +-
 src/evaluate.c                                | 59 +++++++++++++++----
 .../bogons/nft-f/unhandled_key_type_13_assert |  5 ++
 .../nft-f/unhandled_key_type_13_assert_map    |  5 ++
 .../nft-f/unhandled_key_type_13_assert_vmap   |  5 ++
 5 files changed, 64 insertions(+), 16 deletions(-)
 create mode 100644 tests/shell/testcases/bogons/nft-f/unhandled_key_type_13_assert
 create mode 100644 tests/shell/testcases/bogons/nft-f/unhandled_key_type_13_assert_map
 create mode 100644 tests/shell/testcases/bogons/nft-f/unhandled_key_type_13_assert_vmap

--
2.30.2

[PATCH nft 1/4] evaluate: skip anonymous set optimization for concatenations

[Pablo Neira Ayuso]

Concatenation is only supported with sets. Moreover, stripping of the
set leads to broken ruleset listing, therefore, skip this optimization
for the concatenations.

Fixes: fa17b17ea74a ("evaluate: revisit anonymous set with single element optimization")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/evaluate.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 41524eef12b7..6405d55647fa 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -2548,15 +2548,17 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 		return expr_binary_error(ctx->msgs, right, left,
 					 "Cannot be used with right hand side constant value");
 
-	switch (rel->op) {
-	case OP_EQ:
-	case OP_IMPLICIT:
-	case OP_NEQ:
-		if (right->etype == EXPR_SET && right->size == 1)
-			optimize_singleton_set(rel, &right);
-		break;
-	default:
-		break;
+	if (left->etype != EXPR_CONCAT) {
+		switch (rel->op) {
+		case OP_EQ:
+		case OP_IMPLICIT:
+		case OP_NEQ:
+			if (right->etype == EXPR_SET && right->size == 1)
+				optimize_singleton_set(rel, &right);
+			break;
+		default:
+			break;
+		}
 	}
 
 	switch (rel->op) {
-- 
2.30.2

[PATCH nft 2/4] evaluate: do not fetch next expression on runaway number of concatenation components

[Pablo Neira Ayuso]

If this is the last expression, then the runaway flag is set on and
evaluation bails in the next iteration, do not fetch next list element
which refers to the list head.

I found this by code inspection, I could not trigger any crash with this
one.

Fixes: ae1d54d1343f ("evaluate: do not crash on runaway number of concatenation components")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/evaluate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 6405d55647fa..8ef1b5e39bdc 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1621,8 +1621,8 @@ static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
 		if (key && expressions) {
 			if (list_is_last(&key->list, expressions))
 				runaway = true;
-
-			key = list_next_entry(key, list);
+			else
+				key = list_next_entry(key, list);
 		}
 
 		ctx->inner_desc = NULL;
-- 
2.30.2

[PATCH nft 3/4] evaluate: bail out if anonymous concat set defines a non concat expression

[Pablo Neira Ayuso]

Iterate over the element list in the anonymous set to validate that all
expressions are concatenations, otherwise bail out.

  ruleset.nft:3:46-53: Error: expression is not a concatenation
               ip protocol . th dport vmap { tcp / 22 : accept, tcp . 80 : drop}
                                             ^^^^^^^^

This is based on a patch from Florian Westphal.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/evaluate.c                                | 35 ++++++++++++++++++-
 .../bogons/nft-f/unhandled_key_type_13_assert |  5 +++
 .../nft-f/unhandled_key_type_13_assert_map    |  5 +++
 .../nft-f/unhandled_key_type_13_assert_vmap   |  5 +++
 4 files changed, 49 insertions(+), 1 deletion(-)
 create mode 100644 tests/shell/testcases/bogons/nft-f/unhandled_key_type_13_assert
 create mode 100644 tests/shell/testcases/bogons/nft-f/unhandled_key_type_13_assert_map
 create mode 100644 tests/shell/testcases/bogons/nft-f/unhandled_key_type_13_assert_vmap

diff --git a/src/evaluate.c b/src/evaluate.c
index 8ef1b5e39bdc..b79268e82e4f 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -94,6 +94,7 @@ static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
 	struct cmd *cmd;
 	struct set *set;
 	struct handle h;
+	int err;
 
 	if (set_is_datamap(expr->set_flags))
 		key_fix_dtype_byteorder(key);
@@ -118,7 +119,15 @@ static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
 		list_add_tail(&cmd->list, &ctx->cmd->list);
 	}
 
-	set_evaluate(ctx, set);
+	err = set_evaluate(ctx, set);
+	if (err < 0) {
+		list_del(&cmd->list);
+		if (set->flags & NFT_SET_MAP)
+			cmd->set->init = NULL;
+
+		cmd_free(cmd);
+		return NULL;
+	}
 
 	return set_ref_expr_alloc(&expr->location, set);
 }
@@ -2038,6 +2047,8 @@ static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr)
 		mappings = implicit_set_declaration(ctx, "__map%d",
 						    key, data,
 						    mappings);
+		if (!mappings)
+			return -1;
 
 		if (ectx.len && mappings->set->data->len != ectx.len)
 			BUG("%d vs %d\n", mappings->set->data->len, ectx.len);
@@ -2609,6 +2620,9 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 				implicit_set_declaration(ctx, "__set%d",
 							 expr_get(left), NULL,
 							 right);
+			if (!right)
+				return -1;
+
 			/* fall through */
 		case EXPR_SET_REF:
 			if (rel->left->etype == EXPR_CT &&
@@ -3253,6 +3267,8 @@ static int stmt_evaluate_meter(struct eval_ctx *ctx, struct stmt *stmt)
 
 	setref = implicit_set_declaration(ctx, stmt->meter.name,
 					  expr_get(key), NULL, set);
+	if (!setref)
+		return -1;
 
 	setref->set->desc.size = stmt->meter.size;
 	stmt->meter.set = setref;
@@ -4532,6 +4548,8 @@ static int stmt_evaluate_objref_map(struct eval_ctx *ctx, struct stmt *stmt)
 
 		mappings = implicit_set_declaration(ctx, "__objmap%d",
 						    key, NULL, mappings);
+		if (!mappings)
+			return -1;
 		mappings->set->objtype  = stmt->objref.type;
 
 		map->mappings = mappings;
@@ -4865,6 +4883,21 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
 		set->flags |= NFT_SET_CONCAT;
 	}
 
+	if (set_is_anonymous(set->flags) && set->key->etype == EXPR_CONCAT) {
+		struct expr *i;
+
+		list_for_each_entry(i, &set->init->expressions, list) {
+			if ((i->etype == EXPR_SET_ELEM &&
+			     i->key->etype != EXPR_CONCAT &&
+			     i->key->etype != EXPR_SET_ELEM_CATCHALL) ||
+			    (i->etype == EXPR_MAPPING &&
+			     i->left->etype == EXPR_SET_ELEM &&
+			     i->left->key->etype != EXPR_CONCAT &&
+			     i->left->key->etype != EXPR_SET_ELEM_CATCHALL))
+				return expr_error(ctx->msgs, i, "expression is not a concatenation");
+		}
+	}
+
 	if (set_is_datamap(set->flags)) {
 		if (set->data == NULL)
 			return set_error(ctx, set, "map definition does not "
diff --git a/tests/shell/testcases/bogons/nft-f/unhandled_key_type_13_assert b/tests/shell/testcases/bogons/nft-f/unhandled_key_type_13_assert
new file mode 100644
index 000000000000..35eecf607230
--- /dev/null
+++ b/tests/shell/testcases/bogons/nft-f/unhandled_key_type_13_assert
@@ -0,0 +1,5 @@
+table ip x {
+	chain y {
+		ip protocol . th dport { tcp / 22, udp . 67 }
+	}
+}
diff --git a/tests/shell/testcases/bogons/nft-f/unhandled_key_type_13_assert_map b/tests/shell/testcases/bogons/nft-f/unhandled_key_type_13_assert_map
new file mode 100644
index 000000000000..3da16ce15886
--- /dev/null
+++ b/tests/shell/testcases/bogons/nft-f/unhandled_key_type_13_assert_map
@@ -0,0 +1,5 @@
+table ip x {
+	chain y {
+		meta mark set ip protocol . th dport map { tcp / 22 : 1234, udp . 67 : 1234 }
+	}
+}
diff --git a/tests/shell/testcases/bogons/nft-f/unhandled_key_type_13_assert_vmap b/tests/shell/testcases/bogons/nft-f/unhandled_key_type_13_assert_vmap
new file mode 100644
index 000000000000..f4dc273fdb85
--- /dev/null
+++ b/tests/shell/testcases/bogons/nft-f/unhandled_key_type_13_assert_vmap
@@ -0,0 +1,5 @@
+table ip x {
+	chain y {
+		ip protocol . th dport vmap { tcp / 22 : accept, udp . 67 : drop }
+	}
+}
-- 
2.30.2

[PATCH nft 4/4] Revert "datatype: do not assert when value exceeds expected width"

[Pablo Neira Ayuso]

 # nft -f ruleset.nft
 ruleset.nft:3:28-35: Error: expression is not a concatenation
                ip protocol . th dport { tcp / 22,  }
                                         ^^^^^^^^

Therefore, a852022d719e ("datatype: do not assert when value exceeds
expected width") not needed anymore after two previous fixes.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/datatype.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/datatype.c b/src/datatype.c
index 099e7580bd6c..3b19ae8ef52d 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -715,8 +715,7 @@ const struct datatype ip6addr_type = {
 static void inet_protocol_type_print(const struct expr *expr,
 				      struct output_ctx *octx)
 {
-	if (!nft_output_numeric_proto(octx) &&
-	    mpz_cmp_ui(expr->value, UINT8_MAX) <= 0) {
+	if (!nft_output_numeric_proto(octx)) {
 		char name[NFT_PROTONAME_MAXSIZE];
 
 		if (nft_getprotobynumber(mpz_get_uint8(expr->value), name, sizeof(name))) {
@@ -797,8 +796,7 @@ static void inet_service_print(const struct expr *expr, struct output_ctx *octx)
 
 void inet_service_type_print(const struct expr *expr, struct output_ctx *octx)
 {
-	if (nft_output_service(octx) &&
-	    mpz_cmp_ui(expr->value, UINT16_MAX) <= 0) {
+	if (nft_output_service(octx)) {
 		inet_service_print(expr, octx);
 		return;
 	}
-- 
2.30.2

Re: [PATCH nft 4/4] Revert "datatype: do not assert when value exceeds expected width"

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>  # nft -f ruleset.nft
>  ruleset.nft:3:28-35: Error: expression is not a concatenation
>                 ip protocol . th dport { tcp / 22,  }
>                                          ^^^^^^^^
> 
> Therefore, a852022d719e ("datatype: do not assert when value exceeds
> expected width") not needed anymore after two previous fixes.

We can't rely on the expression soup coming from nftables.

Re: [PATCH nft 4/4] Revert "datatype: do not assert when value exceeds expected width"

[Pablo Neira Ayuso]

On Wed, Jan 10, 2024 at 11:57:38PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >  # nft -f ruleset.nft
> >  ruleset.nft:3:28-35: Error: expression is not a concatenation
> >                 ip protocol . th dport { tcp / 22,  }
> >                                          ^^^^^^^^
> > 
> > Therefore, a852022d719e ("datatype: do not assert when value exceeds
> > expected width") not needed anymore after two previous fixes.
> 
> We can't rely on the expression soup coming from nftables.

I can keep this patch then, no problem.

Or you mean this series is botched? Please elaborate :)