nftables; key update with symbolic values/immediates

nftables; key update with symbolic values/immediates

[Florian Westphal]

Hello.

Consider following example:

table ip t {
	set s {
		type ipv4_addr . ipv4_addr . inet_service
		size 65535
		flags dynamic, timeout
		timeout 3h
	}

	chain c1 {
		update @s { ip saddr . 10.180.0.4 . 80 }
	}

	chain c2 {
		ip saddr . 1.2.3.4 . 80 @s goto c1
	}
}

This doesn't work:
:13:14-20: Error: Can't parse symbolic invalid expressions
ip saddr . 1.2.3.4 . 80 @s goto c1

Problem is that expr_evaluate_relational() first evaluates
the lhs, so by the time concat evaluation encounters '1.2.3.4'
symbol there is nothing that would hint at what datatype that is.

For this specific case, its possible to first evaluate the rhs, i.e.:

--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -2336,8 +2336,15 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
        struct expr *range;
        int ret;
 
+       right = rel->right;
+       if (right->etype == EXPR_SYMBOL &&
+           right->symtype == SYMBOL_SET &&
+           expr_evaluate(ctx, &rel->right) < 0)
+               return -1;
+

This populates ectx->key and thus allows to infer the symbolic data
type:

1485                 if (key) {
1486                         tmp = key->dtype;
1487                         dsize = key->len;
1488                         bo = key->byteorder;
1489                         off--;
1490                 } else if (dtype == NULL || off == 0) {
1491                         tmp = datatype_lookup(TYPE_INVALID);

line 1486ff.  With unmodified nft, this hits the 'dtype == NULL' path
and decoding "1.2.3.4" fails.

What do you think?  If you think this is fine I can work on this,
above patch makes nft parse the example above, it needs more work on
delinarization path.

Re: nftables; key update with symbolic values/immediates

[Pablo Neira Ayuso]

Hi Florian,

On Tue, May 23, 2023 at 07:29:31PM +0200, Florian Westphal wrote:
> Hello.
> 
> Consider following example:
> 
> table ip t {
> 	set s {
> 		type ipv4_addr . ipv4_addr . inet_service
> 		size 65535
> 		flags dynamic, timeout
> 		timeout 3h
> 	}
> 
> 	chain c1 {
> 		update @s { ip saddr . 10.180.0.4 . 80 }
> 	}
> 
> 	chain c2 {
> 		ip saddr . 1.2.3.4 . 80 @s goto c1
> 	}
> }
> 
> This doesn't work:
> :13:14-20: Error: Can't parse symbolic invalid expressions
> ip saddr . 1.2.3.4 . 80 @s goto c1
> 
> Problem is that expr_evaluate_relational() first evaluates
> the lhs, so by the time concat evaluation encounters '1.2.3.4'
> symbol there is nothing that would hint at what datatype that is.
> 
> For this specific case, its possible to first evaluate the rhs, i.e.:
> 
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -2336,8 +2336,15 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
>         struct expr *range;
>         int ret;
>  
> +       right = rel->right;
> +       if (right->etype == EXPR_SYMBOL &&
> +           right->symtype == SYMBOL_SET &&
> +           expr_evaluate(ctx, &rel->right) < 0)
> +               return -1;
> +
> 
> This populates ectx->key and thus allows to infer the symbolic data
> type:
> 
> 1485                 if (key) {
> 1486                         tmp = key->dtype;
> 1487                         dsize = key->len;
> 1488                         bo = key->byteorder;
> 1489                         off--;
> 1490                 } else if (dtype == NULL || off == 0) {
> 1491                         tmp = datatype_lookup(TYPE_INVALID);
> 
> line 1486ff.  With unmodified nft, this hits the 'dtype == NULL' path
> and decoding "1.2.3.4" fails.
> 
> What do you think?  If you think this is fine I can work on this,
> above patch makes nft parse the example above, it needs more work on
> delinarization path.

If you have a use-case for this, go ahead.

Is there any other issue you can forecast on the delinearize path? Or
is it just that the code to handle this is missing?

Thanks.

Re: nftables; key update with symbolic values/immediates

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> If you have a use-case for this, go ahead.

Thanks, will do.

> Is there any other issue you can forecast on the delinearize path? Or
> is it just that the code to handle this is missing?

Its just missing, I hope it doesn't require any insane stunts.

[PATCH nft] src: permit use of constant values in set lookup keys

[Florian Westphal]

Given:

set s { type ipv4_addr . ipv4_addr . inet_service .. } rule like

   add rule ip saddr . 1.2.3.4 . 80 @s goto c1

fails with: "Error: Can't parse symbolic invalid expressions".

This fails because the relational expression first evaluates
the left hand side, so when concat evaluation sees '1.2.3.4'
no key context is available.

Check if the RHS is a set reference, and, if so, evaluate
the right hand side.

This sets a pointer to the set key in the evaluation context
structure which then makes the concat evaluation step parse
1.2.3.4 and 80 as ipv4 address and 16bit port number.

On delinearization, extend relop postprocessing to
copy the datatype from the rhs (set reference, has
proper datatype according to set->key) to the lhs (concat
expression).

Note that even with this change following isn't supported:
set s { type inet_service ...
add rule 80 @s

Because of '!expr_is_constant()' check. I decided to leave
that in place for now because I don't see the use-case for
a fixed-value lookup from the packet path.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/evaluate.c                                   |  6 ++++++
 src/netlink_delinearize.c                        |  9 +++++++++
 .../testcases/sets/dumps/type_set_symbol.nft     | 16 ++++++++++++++++
 tests/shell/testcases/sets/type_set_symbol       |  6 ++++++
 4 files changed, 37 insertions(+)
 create mode 100644 tests/shell/testcases/sets/dumps/type_set_symbol.nft
 create mode 100755 tests/shell/testcases/sets/type_set_symbol

diff --git a/src/evaluate.c b/src/evaluate.c
index 17a437bd6530..50f1496c7821 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -2336,6 +2336,12 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 	struct expr *range;
 	int ret;
 
+	right = rel->right;
+	if (right->etype == EXPR_SYMBOL &&
+	    right->symtype == SYMBOL_SET &&
+	    expr_evaluate(ctx, &rel->right) < 0)
+		return -1;
+
 	if (expr_evaluate(ctx, &rel->left) < 0)
 		return -1;
 	left = rel->left;
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 935a6667a1c7..9241f46622ff 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -2839,6 +2839,15 @@ static void expr_postprocess(struct rule_pp_ctx *ctx, struct expr **exprp)
 		case EXPR_PAYLOAD:
 			payload_match_postprocess(ctx, expr, expr->left);
 			return;
+		case EXPR_CONCAT:
+			if (expr->right->etype == EXPR_SET_REF) {
+				assert(expr->left->dtype == &invalid_type);
+				assert(expr->right->dtype != &invalid_type);
+
+				datatype_set(expr->left, expr->right->dtype);
+			}
+			expr_postprocess(ctx, &expr->left);
+			break;
 		default:
 			expr_postprocess(ctx, &expr->left);
 			break;
diff --git a/tests/shell/testcases/sets/dumps/type_set_symbol.nft b/tests/shell/testcases/sets/dumps/type_set_symbol.nft
new file mode 100644
index 000000000000..21209f6d8b19
--- /dev/null
+++ b/tests/shell/testcases/sets/dumps/type_set_symbol.nft
@@ -0,0 +1,16 @@
+table ip t {
+	set s1 {
+		type ipv4_addr . ipv4_addr . inet_service
+		size 65535
+		flags dynamic,timeout
+		timeout 3h
+	}
+
+	chain c1 {
+		update @s1 { ip saddr . 10.180.0.4 . 80 }
+	}
+
+	chain c2 {
+		ip saddr . 1.2.3.4 . 80 @s1 goto c1
+	}
+}
diff --git a/tests/shell/testcases/sets/type_set_symbol b/tests/shell/testcases/sets/type_set_symbol
new file mode 100755
index 000000000000..07820b7c4fdd
--- /dev/null
+++ b/tests/shell/testcases/sets/type_set_symbol
@@ -0,0 +1,6 @@
+#!/bin/bash
+
+set -e
+dumpfile=$(dirname $0)/dumps/$(basename $0).nft
+
+$NFT -f "$dumpfile"
-- 
2.39.3