[PATCH v2 nft 0/3] set related parser fixes

[PATCH v2 nft 0/3] set related parser fixes

[Florian Westphal]

This patchset makes parsing and evaluation of sets and
set elements more robust.

See individual patches and the included bogus-input-tests
for details.

Florian Westphal (3):
  intervals: allow low-level interval code to return errors
  src: do not merge a set with a erroneous one
  evaluate: don't assert if set->data is NULL

 include/rule.h                                |  2 +
 src/evaluate.c                                | 31 +++++++++-
 src/intervals.c                               | 62 ++++++++++++++-----
 .../expr_evaluate_mapping_no_data_assert      |  4 ++
 .../nft-f/invalid_range_expr_type_binop       | 12 ++++
 .../bogons/nft-f/unhandled_key_type_13_assert |  5 ++
 6 files changed, 99 insertions(+), 17 deletions(-)
 create mode 100644 tests/shell/testcases/bogons/nft-f/expr_evaluate_mapping_no_data_assert
 create mode 100644 tests/shell/testcases/bogons/nft-f/invalid_range_expr_type_binop
 create mode 100644 tests/shell/testcases/bogons/nft-f/unhandled_key_type_13_assert

-- 
2.41.0

[PATCH v2 nft 1/3] intervals: allow low-level interval code to return errors

[Florian Westphal]

The low-level code cannot return errors, it lacks the list_head to
enqueue error messages to and functions do not return an errnor number.

Problem is that this code resolves this via assert()s, but it turns out
that with malformed rulesets this can trigger at will, evaluation loop
is not enough to protect us.

Change this and replace asserts with actual error messages so
libnftables can handle this without exiting.

Before:
BUG: unhandled key type 13
nft: src/intervals.c:64: setelem_expr_to_range: Assertion `0' failed.

After:
unhandled_key_type_13:4:38-45: Error: unexpected type concat
 ip protocol . th dport { tcp / 22, udp . 67 }
                                    ^^^^^^^^

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/evaluate.c                                | 14 ++++-
 src/intervals.c                               | 59 ++++++++++++++-----
 .../bogons/nft-f/unhandled_key_type_13_assert |  5 ++
 3 files changed, 63 insertions(+), 15 deletions(-)
 create mode 100644 tests/shell/testcases/bogons/nft-f/unhandled_key_type_13_assert

diff --git a/src/evaluate.c b/src/evaluate.c
index 41524eef12b7..eac9267a0107 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,9 @@ 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)
+		return NULL;
 
 	return set_ref_expr_alloc(&expr->location, set);
 }
@@ -2038,6 +2041,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);
@@ -2607,6 +2612,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 &&
@@ -3251,6 +3259,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;
@@ -4530,6 +4540,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;
diff --git a/src/intervals.c b/src/intervals.c
index 5a88a8eb20bd..ea39dbb80665 100644
--- a/src/intervals.c
+++ b/src/intervals.c
@@ -13,9 +13,9 @@
 #include <intervals.h>
 #include <rule.h>
 
-static void set_to_range(struct expr *init);
+static int set_to_range(struct list_head *msgs, struct expr *init);
 
-static void setelem_expr_to_range(struct expr *expr)
+static int setelem_expr_to_range(struct list_head *msgs, struct expr *expr)
 {
 	unsigned char data[sizeof(struct in6_addr) * BITS_PER_BYTE];
 	struct expr *key, *value;
@@ -61,8 +61,10 @@ static void setelem_expr_to_range(struct expr *expr)
 		expr->key = key;
 		break;
 	default:
-		BUG("unhandled key type %d\n", expr->key->etype);
+		return expr_error(msgs, expr->key, "unexpected type %s", expr_name(expr->key));
 	}
+
+	return 0;
 }
 
 struct set_automerge_ctx {
@@ -125,24 +127,34 @@ static bool merge_ranges(struct set_automerge_ctx *ctx,
 	return false;
 }
 
-static void set_sort_splice(struct expr *init, struct set *set)
+static int set_sort_splice(struct list_head *msgs,
+			   struct expr *init, struct set *set)
 {
 	struct set *existing_set = set->existing_set;
+	int err;
+
+	err = set_to_range(msgs, init);
+	if (err)
+		return err;
 
-	set_to_range(init);
 	list_expr_sort(&init->expressions);
 
 	if (!existing_set)
-		return;
+		return 0;
 
 	if (existing_set->init) {
-		set_to_range(existing_set->init);
+		err = set_to_range(msgs, existing_set->init);
+		if (err)
+			return err;
+
 		list_splice_sorted(&existing_set->init->expressions,
 				   &init->expressions);
 		init_list_head(&existing_set->init->expressions);
 	} else {
 		existing_set->init = set_expr_alloc(&internal_location, set);
 	}
+
+	return 0;
 }
 
 static void setelem_automerge(struct set_automerge_ctx *ctx)
@@ -220,14 +232,19 @@ static struct expr *interval_expr_key(struct expr *i)
 	return elem;
 }
 
-static void set_to_range(struct expr *init)
+static int set_to_range(struct list_head *msgs, struct expr *init)
 {
 	struct expr *i, *elem;
+	int err = 0;
 
 	list_for_each_entry(i, &init->expressions, list) {
 		elem = interval_expr_key(i);
-		setelem_expr_to_range(elem);
+		err = setelem_expr_to_range(msgs, elem);
+		if (err)
+			return err;
 	}
+
+	return err;
 }
 
 int set_automerge(struct list_head *msgs, struct cmd *cmd, struct set *set,
@@ -242,14 +259,21 @@ int set_automerge(struct list_head *msgs, struct cmd *cmd, struct set *set,
 	struct expr *i, *next, *clone;
 	struct cmd *purge_cmd;
 	struct handle h = {};
+	int err;
 
 	if (set->flags & NFT_SET_MAP) {
-		set_to_range(init);
+		err = set_to_range(msgs, init);
+
+		if (err < 0)
+			return err;
+
 		list_expr_sort(&init->expressions);
 		return 0;
 	}
 
-	set_sort_splice(init, set);
+	err = set_sort_splice(msgs, init, set);
+	if (err)
+		return err;
 
 	ctx.purge = set_expr_alloc(&internal_location, set);
 
@@ -483,12 +507,17 @@ int set_delete(struct list_head *msgs, struct cmd *cmd, struct set *set,
 	LIST_HEAD(del_list);
 	int err;
 
-	set_to_range(init);
+	err = set_to_range(msgs, init);
+	if (err)
+		return err;
+
 	if (set->automerge)
 		automerge_delete(msgs, set, init, debug_mask);
 
 	if (existing_set->init) {
-		set_to_range(existing_set->init);
+		err = set_to_range(msgs, existing_set->init);
+		if (err)
+			return err;
 	} else {
 		existing_set->init = set_expr_alloc(&internal_location, set);
 	}
@@ -616,7 +645,9 @@ int set_overlap(struct list_head *msgs, struct set *set, struct expr *init)
 	struct expr *i, *n, *clone;
 	int err;
 
-	set_sort_splice(init, set);
+	err = set_sort_splice(msgs, init, set);
+	if (err)
+		return err;
 
 	err = setelem_overlap(msgs, set, init);
 
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 }
+	}
+}
-- 
2.41.0

[PATCH v2 nft 2/3] src: do not merge a set with a erroneous one

[Florian Westphal]

The included sample causes a crash because we attempt to
range-merge a prefix expression with a symbolic expression.

This happens because the first set is evaluated, the symbol
expr evaluation fails, because the symbolic name cannot be resolved.

nft queues error message
("Could not resolve service: Servname not supported for ai_socktype")
*BUT CONTINUES PROCESSING* of the remaining ruleset.

It then encounters the same set definition again and merges the
new content with the preceeding one.

But the first set structure is dodgy, it still contains the
unresolved symbolic expression.

That then makes nft crash (assert) in the set internals.

There are various different incarnations of this issue, but the low
level set processing code does not allow for any partially transformed
expressions to still remain.

Before:
nft --check -f tests/shell/testcases/bogons/nft-f/invalid_range_expr_type_binop
BUG: invalid range expression type binop
nft: src/expression.c:1479: range_expr_value_low: Assertion `0' failed.

After:
nft --check -f tests/shell/testcases/bogons/nft-f/invalid_range_expr_type_binop
invalid_range_expr_type_binop:4:18-25: Error: Could not resolve hostname: Name or service not known
elements = { 1&.141.0.1 - 192.168.0.2}
             ^^^^^^^^

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/rule.h                                       |  2 ++
 src/evaluate.c                                       |  4 +++-
 src/intervals.c                                      |  3 +++
 .../bogons/nft-f/invalid_range_expr_type_binop       | 12 ++++++++++++
 4 files changed, 20 insertions(+), 1 deletion(-)
 create mode 100644 tests/shell/testcases/bogons/nft-f/invalid_range_expr_type_binop

diff --git a/include/rule.h b/include/rule.h
index 6835fe069165..02fe2e1665e3 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -329,6 +329,7 @@ void rule_stmt_insert_at(struct rule *rule, struct stmt *nstmt,
  * @policy:	set mechanism policy
  * @automerge:	merge adjacents and overlapping elements, if possible
  * @comment:	comment
+ * @errors:	expr evaluation errors seen
  * @desc.size:		count of set elements
  * @desc.field_len:	length of single concatenated fields, bytes
  * @desc.field_count:	count of concatenated fields
@@ -353,6 +354,7 @@ struct set {
 	bool			root;
 	bool			automerge;
 	bool			key_typeof_valid;
+	bool			errors;
 	const char		*comment;
 	struct {
 		uint32_t	size;
diff --git a/src/evaluate.c b/src/evaluate.c
index eac9267a0107..4e9a95ad4c9d 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -4800,8 +4800,10 @@ static int elems_evaluate(struct eval_ctx *ctx, struct set *set)
 
 		__expr_set_context(&ctx->ectx, set->key->dtype,
 				   set->key->byteorder, set->key->len, 0);
-		if (expr_evaluate(ctx, &set->init) < 0)
+		if (expr_evaluate(ctx, &set->init) < 0) {
+			set->errors = true;
 			return -1;
+		}
 		if (set->init->etype != EXPR_SET)
 			return expr_error(ctx->msgs, set->init, "Set %s: Unexpected initial type %s, missing { }?",
 					  set->handle.set.name, expr_name(set->init));
diff --git a/src/intervals.c b/src/intervals.c
index ea39dbb80665..85b44d105402 100644
--- a/src/intervals.c
+++ b/src/intervals.c
@@ -142,6 +142,9 @@ static int set_sort_splice(struct list_head *msgs,
 	if (!existing_set)
 		return 0;
 
+	if (existing_set->errors)
+		return -1;
+
 	if (existing_set->init) {
 		err = set_to_range(msgs, existing_set->init);
 		if (err)
diff --git a/tests/shell/testcases/bogons/nft-f/invalid_range_expr_type_binop b/tests/shell/testcases/bogons/nft-f/invalid_range_expr_type_binop
new file mode 100644
index 000000000000..514d6ffe1319
--- /dev/null
+++ b/tests/shell/testcases/bogons/nft-f/invalid_range_expr_type_binop
@@ -0,0 +1,12 @@
+table ip x {
+	map z {
+		type ipv4_addr : ipv4_addr
+		elements = { 1&.141.0.1 - 192.168.0.2}
+	}
+
+	map z {
+		type ipv4_addr : ipv4_addr
+		flags interval
+		elements = { 10.141.0.0, * : 192.168.0.4 }
+	}
+}
-- 
2.41.0

[PATCH v2 nft 3/3] evaluate: don't assert if set->data is NULL

[Florian Westphal]

For the objref map case, set->data is only non-null if set evaluation
completed successfully.

Before:
nft: src/evaluate.c:2115: expr_evaluate_mapping: Assertion `set->data != NULL' failed.

After:
expr_evaluate_mapping_no_data_assert:1:5-5: Error: No such file or directory
map m p {
    ^

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/evaluate.c                                      | 13 ++++++++++++-
 .../nft-f/expr_evaluate_mapping_no_data_assert      |  4 ++++
 2 files changed, 16 insertions(+), 1 deletion(-)
 create mode 100644 tests/shell/testcases/bogons/nft-f/expr_evaluate_mapping_no_data_assert

diff --git a/src/evaluate.c b/src/evaluate.c
index 4e9a95ad4c9d..582877ecea9a 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -2144,6 +2144,10 @@ static int expr_evaluate_mapping(struct eval_ctx *ctx, struct expr **expr)
 	if (!set_is_map(set->flags))
 		return set_error(ctx, set, "set is not a map");
 
+	/* set already has more known issues, do not evaluate further */
+	if (set->errors)
+		return -1;
+
 	expr_set_context(&ctx->ectx, set->key->dtype, set->key->len);
 	if (expr_evaluate(ctx, &mapping->left) < 0)
 		return -1;
@@ -5387,12 +5391,17 @@ static int table_evaluate(struct eval_ctx *ctx, struct table *table)
 
 static int cmd_evaluate_add(struct eval_ctx *ctx, struct cmd *cmd)
 {
+	int err = -1;
+
 	switch (cmd->obj) {
 	case CMD_OBJ_ELEMENTS:
 		return setelem_evaluate(ctx, cmd);
 	case CMD_OBJ_SET:
 		handle_merge(&cmd->set->handle, &cmd->handle);
-		return set_evaluate(ctx, cmd->set);
+		err = set_evaluate(ctx, cmd->set);
+		if (err)
+			cmd->set->errors = true;
+		break;
 	case CMD_OBJ_SETELEMS:
 		return elems_evaluate(ctx, cmd->set);
 	case CMD_OBJ_RULE:
@@ -5418,6 +5427,8 @@ static int cmd_evaluate_add(struct eval_ctx *ctx, struct cmd *cmd)
 	default:
 		BUG("invalid command object type %u\n", cmd->obj);
 	}
+
+	return err;
 }
 
 static void table_del_cache(struct eval_ctx *ctx, struct cmd *cmd)
diff --git a/tests/shell/testcases/bogons/nft-f/expr_evaluate_mapping_no_data_assert b/tests/shell/testcases/bogons/nft-f/expr_evaluate_mapping_no_data_assert
new file mode 100644
index 000000000000..34d3df61f334
--- /dev/null
+++ b/tests/shell/testcases/bogons/nft-f/expr_evaluate_mapping_no_data_assert
@@ -0,0 +1,4 @@
+map m p {
+	type ipv4_addr : counter
+	elements = { 1.2.3.4 : 1, }
+}
-- 
2.41.0

Re: [PATCH v2 nft 3/3] evaluate: don't assert if set->data is NULL

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> For the objref map case, set->data is only non-null if set evaluation
> completed successfully.
> 
> Before:
> nft: src/evaluate.c:2115: expr_evaluate_mapping: Assertion `set->data != NULL' failed.
> 
> After:
> expr_evaluate_mapping_no_data_assert:1:5-5: Error: No such file or directory
> map m p {
>    ^

This is also the error that one gets with nft 1.0.7. Will add
Fixes: 56c90a2dd2eb ("evaluate: expand sets and maps before evaluation")

to the changelog.