[PATCH nft 0/6] Misc parser fixes

[PATCH nft 0/6] Misc parser fixes

[Florian Westphal]

These patches fix various bugs in the parsing
and evaluation steps.

I added a new 'bogons' test dir to shell, this can be used
to collect invalid inputs that should be rejected instead
of nft exiting with an assertion failure.

Florian Westphal (6):
  json: dccp: remove erroneous const qualifier
  evaluate: do not abort when prefix map has non-map element
  parser: don't assert on scope underflows
  parser: reject zero-length interface names
  parser: reject zero-length interface names in flowtables
  ct timeout: fix 'list object x' vs. 'list objects in table' confusion

 include/rule.h                                |  1 +
 src/cache.c                                   |  1 +
 src/evaluate.c                                | 18 ++++--
 src/parser_bison.y                            | 61 ++++++++++++++-----
 src/parser_json.c                             |  2 +-
 src/rule.c                                    |  1 +
 tests/shell/testcases/bogons/assert_failures  | 12 ++++
 .../nat_prefix_map_with_set_element_assert    |  7 +++
 .../bogons/nft-f/scope_underflow_assert       |  6 ++
 .../nft-f/zero_length_devicename_assert       |  5 ++
 .../zero_length_devicename_flowtable_assert   |  5 ++
 11 files changed, 98 insertions(+), 21 deletions(-)
 create mode 100755 tests/shell/testcases/bogons/assert_failures
 create mode 100644 tests/shell/testcases/bogons/nft-f/nat_prefix_map_with_set_element_assert
 create mode 100644 tests/shell/testcases/bogons/nft-f/scope_underflow_assert
 create mode 100644 tests/shell/testcases/bogons/nft-f/zero_length_devicename_assert
 create mode 100644 tests/shell/testcases/bogons/nft-f/zero_length_devicename_flowtable_assert

-- 
2.39.3

[PATCH nft 1/6] json: dccp: remove erroneous const qualifier

[Florian Westphal]

This causes a clang warning:

parser_json.c:767:6: warning: variable 'opt_type' is uninitialized when used here [-Wuninitialized]
if (opt_type < DCCPOPT_TYPE_MIN || opt_type > DCCPOPT_TYPE_MAX) {
            ^~~~~~~~
... because it deduces the object is readonly.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/parser_json.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index f1cc39505382..c1e15ee186f5 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -759,7 +759,7 @@ static struct expr *json_parse_sctp_chunk_expr(struct json_ctx *ctx,
 static struct expr *json_parse_dccp_option_expr(struct json_ctx *ctx,
 						const char *type, json_t *root)
 {
-	const int opt_type;
+	int opt_type;
 
 	if (json_unpack_err(ctx, root, "{s:i}", "type", &opt_type))
 		return NULL;
-- 
2.39.3

[PATCH nft 2/6] evaluate: do not abort when prefix map has non-map element

[Florian Westphal]

Before:
nft: evaluate.c:1849: __mapping_expr_expand: Assertion `i->etype == EXPR_MAPPING' failed.

after:
Error: expected mapping, not set element
   snat ip prefix to ip saddr map { 10.141.11.0/24 : 192.168.2.0/24, 10.141.12.1 }

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/evaluate.c                                  | 17 +++++++++++++----
 tests/shell/testcases/bogons/assert_failures    | 12 ++++++++++++
 .../nat_prefix_map_with_set_element_assert      |  7 +++++++
 3 files changed, 32 insertions(+), 4 deletions(-)
 create mode 100755 tests/shell/testcases/bogons/assert_failures
 create mode 100644 tests/shell/testcases/bogons/nft-f/nat_prefix_map_with_set_element_assert

diff --git a/src/evaluate.c b/src/evaluate.c
index 00bb8988bd4c..efab28952e32 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1869,12 +1869,21 @@ static void __mapping_expr_expand(struct expr *i)
 	}
 }
 
-static void mapping_expr_expand(struct expr *init)
+static int mapping_expr_expand(struct eval_ctx *ctx)
 {
 	struct expr *i;
 
-	list_for_each_entry(i, &init->expressions, list)
+	if (!set_is_anonymous(ctx->set->flags))
+		return 0;
+
+	list_for_each_entry(i, &ctx->set->init->expressions, list) {
+		if (i->etype != EXPR_MAPPING)
+			return expr_error(ctx->msgs, i,
+					  "expected mapping, not %s", expr_name(i));
 		__mapping_expr_expand(i);
+	}
+
+	return 0;
 }
 
 static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr)
@@ -1955,8 +1964,8 @@ static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr)
 		if (ctx->set->data->flags & EXPR_F_INTERVAL) {
 			ctx->set->data->len *= 2;
 
-			if (set_is_anonymous(ctx->set->flags))
-				mapping_expr_expand(ctx->set->init);
+			if (mapping_expr_expand(ctx))
+				return -1;
 		}
 
 		ctx->set->key->len = ctx->ectx.len;
diff --git a/tests/shell/testcases/bogons/assert_failures b/tests/shell/testcases/bogons/assert_failures
new file mode 100755
index 000000000000..79099427c98a
--- /dev/null
+++ b/tests/shell/testcases/bogons/assert_failures
@@ -0,0 +1,12 @@
+#!/bin/bash
+
+dir=$(dirname $0)/nft-f/
+
+for f in $dir/*; do
+	$NFT --check -f "$f"
+
+	if [ $? -ne 1 ]; then
+		echo "Bogus input file $f did not cause expected error code" 1>&2
+		exit 111
+	fi
+done
diff --git a/tests/shell/testcases/bogons/nft-f/nat_prefix_map_with_set_element_assert b/tests/shell/testcases/bogons/nft-f/nat_prefix_map_with_set_element_assert
new file mode 100644
index 000000000000..18c7edd14c5d
--- /dev/null
+++ b/tests/shell/testcases/bogons/nft-f/nat_prefix_map_with_set_element_assert
@@ -0,0 +1,7 @@
+table ip x {
+	chain y {
+	type nat hook postrouting priority srcnat; policy accept;
+		snat ip prefix to ip saddr map { 10.141.11.0/24 : 192.168.2.0/24, 10.141.12.1 }
+	}
+}
+
-- 
2.39.3

[PATCH nft 3/6] parser: don't assert on scope underflows

[Florian Westphal]

close_scope() gets called from the object destructors;
imbalance can cause us to hit assert().

Before:
nft: parser_bison.y:88: close_scope: Assertion `state->scope > 0' failed.
After:
assertion3:4:7-7: Error: too many levels of nesting jump {
assertion3:5:8-8: Error: too many levels of nesting jump
assertion3:5:9-9: Error: syntax error, unexpected newline, expecting '{'
assertion3:7:1-1: Error: syntax error, unexpected end of file

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/parser_bison.y                                        | 3 +--
 tests/shell/testcases/bogons/nft-f/scope_underflow_assert | 6 ++++++
 2 files changed, 7 insertions(+), 2 deletions(-)
 create mode 100644 tests/shell/testcases/bogons/nft-f/scope_underflow_assert

diff --git a/src/parser_bison.y b/src/parser_bison.y
index 763c1b2dcd61..f5f6bf04d064 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -80,12 +80,11 @@ static int open_scope(struct parser_state *state, struct scope *scope)
 
 static void close_scope(struct parser_state *state)
 {
-	if (state->scope_err) {
+	if (state->scope_err || state->scope == 0) {
 		state->scope_err = false;
 		return;
 	}
 
-	assert(state->scope > 0);
 	state->scope--;
 }
 
diff --git a/tests/shell/testcases/bogons/nft-f/scope_underflow_assert b/tests/shell/testcases/bogons/nft-f/scope_underflow_assert
new file mode 100644
index 000000000000..aee1dcbf9d8a
--- /dev/null
+++ b/tests/shell/testcases/bogons/nft-f/scope_underflow_assert
@@ -0,0 +1,6 @@
+table t {
+	chain c {
+		jump{
+			jump {
+				jump
+
-- 
2.39.3

[PATCH nft 4/6] parser: reject zero-length interface names

[Florian Westphal]

device "" results in an assertion during evaluation.
Before:
nft: expression.c:426: constant_expr_alloc: Assertion `(((len) + (8) - 1) / (8)) > 0' failed.
After:
zero_length_devicename_assert:3:42-49: Error: you cannot set an empty interface name
type filter hook ingress device""lo" priority -1
                         ^^^^^^^^
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/parser_bison.y                            | 36 ++++++++++++++++---
 .../nft-f/zero_length_devicename_assert       |  5 +++
 2 files changed, 36 insertions(+), 5 deletions(-)
 create mode 100644 tests/shell/testcases/bogons/nft-f/zero_length_devicename_assert

diff --git a/src/parser_bison.y b/src/parser_bison.y
index f5f6bf04d064..d5a2f85387cb 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -144,6 +144,33 @@ static bool already_set(const void *attr, const struct location *loc,
 	return true;
 }
 
+static struct expr* ifname_expr_alloc(const struct location *location,
+				      struct list_head *queue,
+				      const char *name)
+{
+	unsigned int length = strlen(name);
+	struct expr *expr;
+
+	if (length == 0) {
+		xfree(name);
+		erec_queue(error(location, "empty interface name"), queue);
+		return NULL;
+	}
+
+	if (length > 16) {
+		xfree(name);
+		erec_queue(error(location, "interface name too long"), queue);
+		return NULL;
+	}
+
+	expr = constant_expr_alloc(location, &ifname_type, BYTEORDER_HOST_ENDIAN,
+				   length * BITS_PER_BYTE, name);
+
+	xfree(name);
+
+	return expr;
+}
+
 #define YYLLOC_DEFAULT(Current, Rhs, N)	location_update(&Current, Rhs, N)
 
 #define symbol_value(loc, str) \
@@ -2664,12 +2691,11 @@ int_num			:	NUM			{ $$ = $1; }
 
 dev_spec		:	DEVICE	string
 			{
-				struct expr *expr;
+				struct expr *expr = ifname_expr_alloc(&@$, state->msgs, $2);
+
+				if (!expr)
+					YYERROR;
 
-				expr = constant_expr_alloc(&@$, &string_type,
-							   BYTEORDER_HOST_ENDIAN,
-							   strlen($2) * BITS_PER_BYTE, $2);
-				xfree($2);
 				$$ = compound_expr_alloc(&@$, EXPR_LIST);
 				compound_expr_add($$, expr);
 
diff --git a/tests/shell/testcases/bogons/nft-f/zero_length_devicename_assert b/tests/shell/testcases/bogons/nft-f/zero_length_devicename_assert
new file mode 100644
index 000000000000..84f330730fce
--- /dev/null
+++ b/tests/shell/testcases/bogons/nft-f/zero_length_devicename_assert
@@ -0,0 +1,5 @@
+table ip x {
+        chain Main_Ingress1 {
+                type filter hook ingress device""lo" priority -1
+	}
+}
-- 
2.39.3

[PATCH nft 5/6] parser: reject zero-length interface names in flowtables

[Florian Westphal]

Previous patch wasn't enough, also disable this for flowtable device lists.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/parser_bison.y                            | 20 +++++++++++--------
 .../zero_length_devicename_flowtable_assert   |  5 +++++
 2 files changed, 17 insertions(+), 8 deletions(-)
 create mode 100644 tests/shell/testcases/bogons/nft-f/zero_length_devicename_flowtable_assert

diff --git a/src/parser_bison.y b/src/parser_bison.y
index d5a2f85387cb..8a559103250e 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -2380,17 +2380,21 @@ flowtable_list_expr	:	flowtable_expr_member
 
 flowtable_expr_member	:	QUOTED_STRING
 			{
-				$$ = constant_expr_alloc(&@$, &string_type,
-							 BYTEORDER_HOST_ENDIAN,
-							 strlen($1) * BITS_PER_BYTE, $1);
-				xfree($1);
+				struct expr *expr = ifname_expr_alloc(&@$, state->msgs, $1);
+
+				if (!expr)
+					YYERROR;
+
+				$$ = expr;
 			}
 			|	STRING
 			{
-				$$ = constant_expr_alloc(&@$, &string_type,
-							 BYTEORDER_HOST_ENDIAN,
-							 strlen($1) * BITS_PER_BYTE, $1);
-				xfree($1);
+				struct expr *expr = ifname_expr_alloc(&@$, state->msgs, $1);
+
+				if (!expr)
+					YYERROR;
+
+				$$ = expr;
 			}
 			|	variable_expr
 			{
diff --git a/tests/shell/testcases/bogons/nft-f/zero_length_devicename_flowtable_assert b/tests/shell/testcases/bogons/nft-f/zero_length_devicename_flowtable_assert
new file mode 100644
index 000000000000..2c3e6c3ffbd2
--- /dev/null
+++ b/tests/shell/testcases/bogons/nft-f/zero_length_devicename_flowtable_assert
@@ -0,0 +1,5 @@
+table t {
+	flowtable f {
+		devices = { """"lo }
+	}
+}
-- 
2.39.3

[PATCH nft 6/6] ct timeout: fix 'list object x' vs. 'list objects in table' confusion

[Florian Westphal]

<empty ruleset>
$ nft list ct timeout table t
Error: No such file or directory
list ct timeout table t
                      ^
This is expected to list all 'ct timeout' objects.
The failure is correct, the table 't' does not exist.

But now lets add one:
$ nft add table t
$ nft list ct timeout  table t
Segmentation fault (core dumped)

... and thats not expected, nothing should be shown
and nft should exit normally.

Because of missing TIMEOUTS command enum, the backend thinks
it should do an object lookup, but as frontend asked for
'list of objects' rather than 'show this object',
handle.obj.name is NULL, which then results in this crash.

Update the command enums so that backend knows what the
frontend asked for.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/rule.h     | 1 +
 src/cache.c        | 1 +
 src/evaluate.c     | 1 +
 src/parser_bison.y | 2 +-
 src/rule.c         | 1 +
 5 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/rule.h b/include/rule.h
index fa3915298750..b360e2614c78 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -645,6 +645,7 @@ enum cmd_obj {
 	CMD_OBJ_FLOWTABLE,
 	CMD_OBJ_FLOWTABLES,
 	CMD_OBJ_CT_TIMEOUT,
+	CMD_OBJ_CT_TIMEOUTS,
 	CMD_OBJ_SECMARK,
 	CMD_OBJ_SECMARKS,
 	CMD_OBJ_CT_EXPECT,
diff --git a/src/cache.c b/src/cache.c
index becfa57fc335..d908ae0ad192 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -370,6 +370,7 @@ static int nft_handle_validate(const struct cmd *cmd, struct list_head *msgs)
 	case CMD_OBJ_CT_HELPER:
 	case CMD_OBJ_CT_HELPERS:
 	case CMD_OBJ_CT_TIMEOUT:
+	case CMD_OBJ_CT_TIMEOUTS:
 	case CMD_OBJ_CT_EXPECT:
 		if (h->table.name &&
 		    strlen(h->table.name) > NFT_NAME_MAXLEN) {
diff --git a/src/evaluate.c b/src/evaluate.c
index efab28952e32..687f9a7b5924 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -5441,6 +5441,7 @@ static int cmd_evaluate_list(struct eval_ctx *ctx, struct cmd *cmd)
 	case CMD_OBJ_FLOWTABLES:
 	case CMD_OBJ_SECMARKS:
 	case CMD_OBJ_SYNPROXYS:
+	case CMD_OBJ_CT_TIMEOUTS:
 		if (cmd->handle.table.name == NULL)
 			return 0;
 		if (!table_cache_find(&ctx->nft->cache.table_cache,
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 8a559103250e..47eb81f70aee 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -4757,7 +4757,7 @@ ct_obj_type		:	HELPER		{ $$ = NFT_OBJECT_CT_HELPER; }
 			;
 
 ct_cmd_type		:	HELPERS		{ $$ = CMD_OBJ_CT_HELPERS; }
-			|	TIMEOUT		{ $$ = CMD_OBJ_CT_TIMEOUT; }
+			|	TIMEOUT		{ $$ = CMD_OBJ_CT_TIMEOUTS; }
 			|	EXPECTATION	{ $$ = CMD_OBJ_CT_EXPECT; }
 			;
 
diff --git a/src/rule.c b/src/rule.c
index 1faa1a27f38e..3704600a87be 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -2351,6 +2351,7 @@ static int do_command_list(struct netlink_ctx *ctx, struct cmd *cmd)
 	case CMD_OBJ_CT_HELPERS:
 		return do_list_obj(ctx, cmd, NFT_OBJECT_CT_HELPER);
 	case CMD_OBJ_CT_TIMEOUT:
+	case CMD_OBJ_CT_TIMEOUTS:
 		return do_list_obj(ctx, cmd, NFT_OBJECT_CT_TIMEOUT);
 	case CMD_OBJ_CT_EXPECT:
 		return do_list_obj(ctx, cmd, NFT_OBJECT_CT_EXPECT);
-- 
2.39.3

Re: [PATCH nft 4/6] parser: reject zero-length interface names

[Pablo Neira Ayuso]

On Mon, Jun 19, 2023 at 10:43:04PM +0200, Florian Westphal wrote:
> device "" results in an assertion during evaluation.
> Before:
> nft: expression.c:426: constant_expr_alloc: Assertion `(((len) + (8) - 1) / (8)) > 0' failed.
> After:
> zero_length_devicename_assert:3:42-49: Error: you cannot set an empty interface name
> type filter hook ingress device""lo" priority -1
>                          ^^^^^^^^
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  src/parser_bison.y                            | 36 ++++++++++++++++---
>  .../nft-f/zero_length_devicename_assert       |  5 +++
>  2 files changed, 36 insertions(+), 5 deletions(-)
>  create mode 100644 tests/shell/testcases/bogons/nft-f/zero_length_devicename_assert
> 
> diff --git a/src/parser_bison.y b/src/parser_bison.y
> index f5f6bf04d064..d5a2f85387cb 100644
> --- a/src/parser_bison.y
> +++ b/src/parser_bison.y
> @@ -144,6 +144,33 @@ static bool already_set(const void *attr, const struct location *loc,
>  	return true;
>  }
>  
> +static struct expr* ifname_expr_alloc(const struct location *location,

Nitpick:

static struct expr *ifname_expr_alloc(const struct location *location,
                   ^

> +				      struct list_head *queue,
> +				      const char *name)
> +{
> +	unsigned int length = strlen(name);
> +	struct expr *expr;
> +
> +	if (length == 0) {
> +		xfree(name);
> +		erec_queue(error(location, "empty interface name"), queue);
> +		return NULL;
> +	}
> +
> +	if (length > 16) {

                     IFNAMSIZ instead of 16?

> +		xfree(name);
> +		erec_queue(error(location, "interface name too long"), queue);
> +		return NULL;
> +	}
> +
> +	expr = constant_expr_alloc(location, &ifname_type, BYTEORDER_HOST_ENDIAN,
> +				   length * BITS_PER_BYTE, name);
> +
> +	xfree(name);
> +
> +	return expr;
> +}
> +
>  #define YYLLOC_DEFAULT(Current, Rhs, N)	location_update(&Current, Rhs, N)
>  
>  #define symbol_value(loc, str) \
> @@ -2664,12 +2691,11 @@ int_num			:	NUM			{ $$ = $1; }
>  
>  dev_spec		:	DEVICE	string
>  			{
> -				struct expr *expr;
> +				struct expr *expr = ifname_expr_alloc(&@$, state->msgs, $2);
> +
> +				if (!expr)
> +					YYERROR;
>  
> -				expr = constant_expr_alloc(&@$, &string_type,
> -							   BYTEORDER_HOST_ENDIAN,
> -							   strlen($2) * BITS_PER_BYTE, $2);
> -				xfree($2);
>  				$$ = compound_expr_alloc(&@$, EXPR_LIST);
>  				compound_expr_add($$, expr);
>  
> diff --git a/tests/shell/testcases/bogons/nft-f/zero_length_devicename_assert b/tests/shell/testcases/bogons/nft-f/zero_length_devicename_assert
> new file mode 100644
> index 000000000000..84f330730fce
> --- /dev/null
> +++ b/tests/shell/testcases/bogons/nft-f/zero_length_devicename_assert
> @@ -0,0 +1,5 @@
> +table ip x {
> +        chain Main_Ingress1 {
> +                type filter hook ingress device""lo" priority -1
> +	}
> +}
> -- 
> 2.39.3
>