[PATCH nft] evaluate: fix expression data corruption

[PATCH nft] evaluate: fix expression data corruption

[Florian Westphal]

Sometimes nftables will segfault when doing error-unwind of the included
afl-generated bogon.

The problem is the unconditional write access to expr->set_flags in
expr_evaluate_map():

   mappings->set_flags |= NFT_SET_MAP;

... but mappings can point to EXPR_VARIABLE (legal), where this will flip
a bit in unused, but allocated memory (i.e., has no effect).

In case of the bogon, mapping is EXPR_RANGE_SYMBOL, and the store can flip
a bit in identifier_range[1], this causes crash when the pointer is freed.

We can't use expr->set_flags unconditionally, so rework this to pass
set_flags as argument and place all read and write accesses in places where
we've made sure we are dealing with EXPR_SET.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/evaluate.c                                | 29 ++++++++++++-------
 .../bogons/nft-f/range_expression_corruption  |  2 ++
 2 files changed, 20 insertions(+), 11 deletions(-)
 create mode 100644 tests/shell/testcases/bogons/nft-f/range_expression_corruption

diff --git a/src/evaluate.c b/src/evaluate.c
index 722c11a23c2d..7fc210fd3b12 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -123,16 +123,16 @@ static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
 	struct set *set;
 	struct handle h;
 
-	if (set_is_datamap(expr->set_flags))
+	if (set_is_datamap(flags))
 		key_fix_dtype_byteorder(key);
 
 	set = set_alloc(&expr->location);
-	set->flags	= expr->set_flags | flags;
+	set->flags	= flags;
 	set->handle.set.name = xstrdup(name);
 	set->key	= key;
 	set->data	= data;
 	set->init	= expr;
-	set->automerge	= set->flags & NFT_SET_INTERVAL;
+	set->automerge	= flags & NFT_SET_INTERVAL;
 
 	handle_merge(&set->handle, &ctx->cmd->handle);
 
@@ -2117,6 +2117,7 @@ static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr)
 {
 	struct expr *map = *expr, *mappings;
 	struct expr_ctx ectx = ctx->ectx;
+	uint32_t set_flags = NFT_SET_MAP;
 	struct expr *key, *data;
 
 	if (map->map->etype == EXPR_CT &&
@@ -2145,11 +2146,14 @@ static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr)
 
 	ctx->stmt_len = 0;
 	mappings = map->mappings;
-	mappings->set_flags |= NFT_SET_MAP;
 
 	switch (map->mappings->etype) {
-	case EXPR_VARIABLE:
+	case EXPR_CONCAT:
+	case EXPR_LIST:
 	case EXPR_SET:
+		set_flags |= mappings->set_flags;
+		/* fallthrough */
+	case EXPR_VARIABLE:
 		if (ctx->ectx.key && ctx->ectx.key->etype == EXPR_CONCAT) {
 			key = expr_clone(ctx->ectx.key);
 		} else {
@@ -2179,7 +2183,7 @@ static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr)
 		mappings = implicit_set_declaration(ctx, "__map%d",
 						    key, data,
 						    mappings,
-						    NFT_SET_ANONYMOUS);
+						    NFT_SET_ANONYMOUS | set_flags);
 		if (!mappings)
 			return -1;
 
@@ -2807,7 +2811,7 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 				implicit_set_declaration(ctx, "__set%d",
 							 expr_get(left), NULL,
 							 right,
-							 NFT_SET_ANONYMOUS);
+							 right->set_flags | NFT_SET_ANONYMOUS);
 			if (!right)
 				return -1;
 
@@ -3529,7 +3533,8 @@ static int stmt_evaluate_meter(struct eval_ctx *ctx, struct stmt *stmt)
 
 		set->set_flags |= NFT_SET_EVAL;
 		setref = implicit_set_declaration(ctx, stmt->meter.name,
-						  expr_get(key), NULL, set, 0);
+						  expr_get(key), NULL, set,
+						  NFT_SET_EVAL | set->set_flags);
 		if (setref)
 			setref->set->desc.size = stmt->meter.size;
 	}
@@ -4742,6 +4747,7 @@ static int stmt_evaluate_map(struct eval_ctx *ctx, struct stmt *stmt)
 static int stmt_evaluate_objref_map(struct eval_ctx *ctx, struct stmt *stmt)
 {
 	struct expr *map = stmt->objref.expr;
+	uint32_t set_flags = NFT_SET_OBJECT;
 	struct expr *mappings;
 	struct expr *key;
 
@@ -4753,11 +4759,12 @@ static int stmt_evaluate_objref_map(struct eval_ctx *ctx, struct stmt *stmt)
 				  "Map expression can not be constant");
 
 	mappings = map->mappings;
-	mappings->set_flags |= NFT_SET_OBJECT;
 
 	switch (map->mappings->etype) {
-	case EXPR_VARIABLE:
 	case EXPR_SET:
+		set_flags |= mappings->set_flags;
+		/* fallthrough */
+	case EXPR_VARIABLE:
 		key = constant_expr_alloc(&stmt->location,
 					  ctx->ectx.dtype,
 					  ctx->ectx.byteorder,
@@ -4765,7 +4772,7 @@ static int stmt_evaluate_objref_map(struct eval_ctx *ctx, struct stmt *stmt)
 
 		mappings = implicit_set_declaration(ctx, "__objmap%d",
 						    key, NULL, mappings,
-						    NFT_SET_ANONYMOUS);
+						    set_flags | NFT_SET_ANONYMOUS);
 		if (!mappings)
 			return -1;
 		mappings->set->objtype  = stmt->objref.type;
diff --git a/tests/shell/testcases/bogons/nft-f/range_expression_corruption b/tests/shell/testcases/bogons/nft-f/range_expression_corruption
new file mode 100644
index 000000000000..b77221bd11a4
--- /dev/null
+++ b/tests/shell/testcases/bogons/nft-f/range_expression_corruption
@@ -0,0 +1,2 @@
+aal	tht@nh,32,3 set ctag| oi to ip
+		p sept ct 		l3proto map  q -u dscp |  ma
\ No newline at end of file
-- 
2.45.3

Re: [PATCH nft] evaluate: fix expression data corruption

[Pablo Neira Ayuso]

On Tue, Mar 11, 2025 at 02:07:03PM +0100, Florian Westphal wrote:
> Sometimes nftables will segfault when doing error-unwind of the included
> afl-generated bogon.
> 
> The problem is the unconditional write access to expr->set_flags in
> expr_evaluate_map():
> 
>    mappings->set_flags |= NFT_SET_MAP;
> 
> ... but mappings can point to EXPR_VARIABLE (legal), where this will flip
> a bit in unused, but allocated memory (i.e., has no effect).
> 
> In case of the bogon, mapping is EXPR_RANGE_SYMBOL, and the store can flip
> a bit in identifier_range[1], this causes crash when the pointer is freed.
> 
> We can't use expr->set_flags unconditionally, so rework this to pass
> set_flags as argument and place all read and write accesses in places where
> we've made sure we are dealing with EXPR_SET.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

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