[PATCH nft v2 0/3] fix icmpv6 id dependeny handling

[PATCH nft v2 0/3] fix icmpv6 id dependeny handling

[Florian Westphal]

v2: in patch 1, make sure set has elements and is anonymous.

Pablo reported following bug:

input: icmpv6 id 1
output: icmpv6 type { echo-request, echo-reply } icmpv6 parameter-problem 65536/16

First patch fixes delinearization to handle this correctly.
Second patch fixes a bug related to dependency removal.
Third patch adds test cases for this bug.

 src/netlink_delinearize.c         |   68 ++++++++++++++++++++++++++++++++++++--
 src/payload.c                     |   61 ++++++++++++++++++++--------------
 tests/py/ip/icmp.t                |    1 
 tests/py/ip/icmp.t.json           |   28 +++++++++++++++
 tests/py/ip/icmp.t.payload.ip     |    9 +++++
 tests/py/ip6/icmpv6.t             |    3 +
 tests/py/ip6/icmpv6.t.json        |   61 ++++++++++++++++++++++++++++++++++
 tests/py/ip6/icmpv6.t.payload.ip6 |   21 +++++++++++
 8 files changed, 225 insertions(+), 27 deletions(-)

[PATCH nft v2 1/3] netlink_delinearize: add missing icmp id/sequence support

[Florian Westphal]

Pablo reports following input and output:
in: icmpv6 id 1
out: icmpv6 type { echo-request, echo-reply } icmpv6 parameter-problem 65536/16

Reason is that icmp fields overlap, decoding of the correct name requires
check of the icmpv6 type.  This only works for equality tests, for
instance

in: icmpv6 type echo-request icmpv6 id 1
will be listed as "icmpv6 id 1" (which is not correct either, since the
input only matches on echo-request).

with this patch, output of 'icmpv6 id 1' is
icmpv6 type { echo-request, echo-reply } icmpv6 id 1

The second problem, the removal of a single check (request OR reply),
is resolved in the followup patch.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 v2: make sure dependency set candidate a) has elements and b) is
 anonymous.

 src/netlink_delinearize.c | 68 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 3 deletions(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 9a1cf3c4f7d9..2785a9490682 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -1819,9 +1819,6 @@ static void payload_match_expand(struct rule_pp_ctx *ctx,
 	enum proto_bases base = left->payload.base;
 	bool stacked;
 
-	if (ctx->pdctx.icmp_type)
-		ctx->pctx.th_dep.icmp.type = ctx->pdctx.icmp_type;
-
 	payload_expr_expand(&list, left, &ctx->pctx);
 
 	list_for_each_entry(left, &list, list) {
@@ -1868,6 +1865,58 @@ static void payload_match_expand(struct rule_pp_ctx *ctx,
 	ctx->stmt = NULL;
 }
 
+static void payload_icmp_check(struct rule_pp_ctx *rctx, struct expr *expr, const struct expr *value)
+{
+	const struct proto_hdr_template *tmpl;
+	const struct proto_desc *desc;
+	uint8_t icmp_type;
+	unsigned int i;
+
+	assert(expr->etype == EXPR_PAYLOAD);
+	assert(value->etype == EXPR_VALUE);
+
+	if (expr->payload.base != PROTO_BASE_TRANSPORT_HDR)
+		return;
+
+	/* icmp(v6) type is 8 bit, if value is smaller or larger, this is not
+	 * a protocol dependency.
+	 */
+	if (expr->len != 8 || value->len != 8 || rctx->pctx.th_dep.icmp.type)
+		return;
+
+	desc = rctx->pctx.protocol[expr->payload.base].desc;
+	if (desc == NULL)
+		return;
+
+	/* not icmp? ignore. */
+	if (desc != &proto_icmp && desc != &proto_icmp6)
+		return;
+
+	assert(desc->base == expr->payload.base);
+
+	icmp_type = mpz_get_uint8(value->value);
+
+	for (i = 1; i < array_size(desc->templates); i++) {
+		tmpl = &desc->templates[i];
+
+		if (tmpl->len == 0)
+			return;
+
+		if (tmpl->offset != expr->payload.offset ||
+		    tmpl->len != expr->len)
+			continue;
+
+		/* Matches but doesn't load a protocol key -> ignore. */
+		if (desc->protocol_key != i)
+			return;
+
+		expr->payload.desc = desc;
+		expr->payload.tmpl = tmpl;
+		rctx->pctx.th_dep.icmp.type = icmp_type;
+		return;
+	}
+}
+
 static void payload_match_postprocess(struct rule_pp_ctx *ctx,
 				      struct expr *expr,
 				      struct expr *payload)
@@ -1883,6 +1932,19 @@ static void payload_match_postprocess(struct rule_pp_ctx *ctx,
 		if (expr->right->etype == EXPR_VALUE) {
 			payload_match_expand(ctx, expr, payload);
 			break;
+		} else if (expr->right->etype == EXPR_SET_REF) {
+			struct set *set = expr->right->set;
+
+			if (set_is_anonymous(set->flags) &&
+			    !list_empty(&set->init->expressions)) {
+				struct expr *elem;
+
+				elem = list_first_entry(&set->init->expressions, struct expr, list);
+
+				if (elem->etype == EXPR_SET_ELEM &&
+				    elem->key->etype == EXPR_VALUE)
+					payload_icmp_check(ctx, payload, elem->key);
+			}
 		}
 		/* Fall through */
 	default:
-- 
2.31.1

[PATCH nft v2 2/3] payload: do not remove icmp echo dependency

[Florian Westphal]

"icmp type echo-request icmp id 2" and "icmp id 2" are not the same,
the latter gains an implicit dependency on both echo-request and
echo-reply.

Change payload dependency tracking to not store dependency in case
the value type is ICMP(6)_ECHO(REPLY).

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/payload.c | 61 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/src/payload.c b/src/payload.c
index cfa952248a15..97b60713e800 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -98,12 +98,16 @@ static void payload_expr_pctx_update(struct proto_ctx *ctx,
 	desc = proto_find_upper(base, proto);
 
 	if (!desc) {
-		if (base == &proto_icmp || base == &proto_icmp6) {
+		if (base == &proto_icmp) {
 			/* proto 0 is ECHOREPLY, just pretend its ECHO.
 			 * Not doing this would need an additional marker
 			 * bit to tell when icmp.type was set.
 			 */
 			ctx->th_dep.icmp.type = proto ? proto : ICMP_ECHO;
+		} else if (base == &proto_icmp6) {
+			if (proto == ICMP6_ECHO_REPLY)
+				proto = ICMP6_ECHO_REQUEST;
+			ctx->th_dep.icmp.type = proto;
 		}
 		return;
 	}
@@ -554,33 +558,39 @@ void payload_dependency_reset(struct payload_dep_ctx *ctx)
 	memset(ctx, 0, sizeof(*ctx));
 }
 
-static uint8_t icmp_get_type(const struct proto_desc *desc, uint8_t value)
+static bool payload_dependency_store_icmp_type(struct payload_dep_ctx *ctx,
+					       const struct stmt *stmt)
 {
-	if (desc == &proto_icmp && value == 0)
-		return ICMP_ECHO;
+	struct expr *dep = stmt->expr;
+	const struct proto_desc *desc;
+	const struct expr *right;
+	uint8_t type;
 
-	return value;
-}
+	if (dep->left->etype != EXPR_PAYLOAD)
+		return false;
 
-static uint8_t icmp_get_dep_type(const struct proto_desc *desc, struct expr *right)
-{
-	if (right->etype == EXPR_VALUE && right->len == BITS_PER_BYTE)
-		return icmp_get_type(desc, mpz_get_uint8(right->value));
+	right = dep->right;
+	if (right->etype != EXPR_VALUE || right->len != BITS_PER_BYTE)
+		return false;
 
-	return 0;
-}
+	desc = dep->left->payload.desc;
+	if (desc == &proto_icmp) {
+		type = mpz_get_uint8(right->value);
 
-static void payload_dependency_store_icmp_type(struct payload_dep_ctx *ctx)
-{
-	struct expr *dep = ctx->pdep->expr;
-	const struct proto_desc *desc;
+		if (type == ICMP_ECHOREPLY)
+			type = ICMP_ECHO;
 
-	if (dep->left->etype != EXPR_PAYLOAD)
-		return;
+		ctx->icmp_type = type;
 
-	desc = dep->left->payload.desc;
-	if (desc == &proto_icmp || desc == &proto_icmp6)
-		ctx->icmp_type = icmp_get_dep_type(dep->left->payload.desc, dep->right);
+		return type == ICMP_ECHO;
+	} else if (desc == &proto_icmp6) {
+		type = mpz_get_uint8(right->value);
+
+		ctx->icmp_type = type;
+		return type == ICMP6_ECHO_REQUEST || type == ICMP6_ECHO_REPLY;
+	}
+
+	return false;
 }
 
 /**
@@ -593,10 +603,13 @@ static void payload_dependency_store_icmp_type(struct payload_dep_ctx *ctx)
 void payload_dependency_store(struct payload_dep_ctx *ctx,
 			      struct stmt *stmt, enum proto_bases base)
 {
-	ctx->pbase = base + 1;
-	ctx->pdep  = stmt;
+	bool ignore_dep = payload_dependency_store_icmp_type(ctx, stmt);
+
+	if (ignore_dep)
+		return;
 
-	payload_dependency_store_icmp_type(ctx);
+	ctx->pdep  = stmt;
+	ctx->pbase = base + 1;
 }
 
 /**
-- 
2.31.1

[PATCH nft v2 3/3] tests: add a icmp-reply only and icmpv6 id test cases

[Florian Westphal]

Check that nft doesn't remove the dependency in these cases:
icmp type echo-reply icmp id 1
("icmp id" matches both echo request and reply).

Add icmpv6 test cases.  These fail without the previous patches:

add rule ip6 test-ip6 input icmpv6 id 1:
 'icmpv6 id 1' mismatches
 'icmpv6 type { echo-request, echo-reply} icmpv6 parameter-problem 65536/16'

add rule ip6 test-ip6 input icmpv6 type echo-reply icmpv6 id 65534':
  'icmpv6 type echo-reply icmpv6 id 65534' mismatches
  'icmpv6 type echo-reply @th,32,16 65534'

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tests/py/ip/icmp.t                |  1 +
 tests/py/ip/icmp.t.json           | 28 ++++++++++++++
 tests/py/ip/icmp.t.payload.ip     |  9 +++++
 tests/py/ip6/icmpv6.t             |  3 ++
 tests/py/ip6/icmpv6.t.json        | 61 +++++++++++++++++++++++++++++++
 tests/py/ip6/icmpv6.t.payload.ip6 | 21 +++++++++++
 6 files changed, 123 insertions(+)

diff --git a/tests/py/ip/icmp.t b/tests/py/ip/icmp.t
index fd89af0dff20..7ddf8b38a538 100644
--- a/tests/py/ip/icmp.t
+++ b/tests/py/ip/icmp.t
@@ -53,6 +53,7 @@ icmp sequence { 33, 55, 67, 88};ok;icmp type { echo-request, echo-reply} icmp se
 icmp sequence != { 33, 55, 67, 88};ok;icmp type { echo-request, echo-reply} icmp sequence != { 33, 55, 67, 88}
 icmp id 1 icmp sequence 2;ok;icmp type { echo-reply, echo-request} icmp id 1 icmp sequence 2
 icmp type { echo-reply, echo-request} icmp id 1 icmp sequence 2;ok
+icmp type echo-reply icmp id 1;ok
 
 icmp mtu 33;ok
 icmp mtu 22-33;ok
diff --git a/tests/py/ip/icmp.t.json b/tests/py/ip/icmp.t.json
index 576335cc63d2..4f0525094cf0 100644
--- a/tests/py/ip/icmp.t.json
+++ b/tests/py/ip/icmp.t.json
@@ -1123,6 +1123,34 @@
     }
 ]
 
+# icmp type echo-reply icmp id 1
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": "echo-reply"
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "id",
+                    "protocol": "icmp"
+                }
+            },
+            "op": "==",
+            "right": 1
+        }
+    }
+]
+
 # icmp mtu 33
 [
     {
diff --git a/tests/py/ip/icmp.t.payload.ip b/tests/py/ip/icmp.t.payload.ip
index 024739c0c3cc..3bc6de3cf717 100644
--- a/tests/py/ip/icmp.t.payload.ip
+++ b/tests/py/ip/icmp.t.payload.ip
@@ -413,6 +413,15 @@ ip
   [ payload load 4b @ transport header + 4 => reg 1 ]
   [ cmp eq reg 1 0x02000100 ]
 
+# icmp type echo-reply icmp id 1
+ip
+  [ meta load l4proto => reg 1 ]
+  [ cmp eq reg 1 0x00000001 ]
+  [ payload load 1b @ transport header + 0 => reg 1 ]
+  [ cmp eq reg 1 0x00000000 ]
+  [ payload load 2b @ transport header + 4 => reg 1 ]
+  [ cmp eq reg 1 0x00000100 ]
+
 # icmp mtu 33
 ip test-ip4 input
   [ meta load l4proto => reg 1 ]
diff --git a/tests/py/ip6/icmpv6.t b/tests/py/ip6/icmpv6.t
index c8d4cffcd9d7..4de6ee2377dd 100644
--- a/tests/py/ip6/icmpv6.t
+++ b/tests/py/ip6/icmpv6.t
@@ -67,6 +67,9 @@ icmpv6 id != 33-45;ok;icmpv6 type { echo-request, echo-reply} icmpv6 id != 33-45
 icmpv6 id {33, 55, 67, 88};ok;icmpv6 type { echo-request, echo-reply} icmpv6 id { 33, 55, 67, 88}
 icmpv6 id != {33, 55, 67, 88};ok;icmpv6 type { echo-request, echo-reply} icmpv6 id != { 33, 55, 67, 88}
 
+icmpv6 id 1;ok;icmpv6 type { echo-request, echo-reply} icmpv6 id 1
+icmpv6 type echo-reply icmpv6 id 65534;ok
+
 icmpv6 sequence 2;ok;icmpv6 type { echo-request, echo-reply} icmpv6 sequence 2
 icmpv6 sequence {3, 4, 5, 6, 7} accept;ok;icmpv6 type { echo-request, echo-reply} icmpv6 sequence { 3, 4, 5, 6, 7} accept
 
diff --git a/tests/py/ip6/icmpv6.t.json b/tests/py/ip6/icmpv6.t.json
index 30d2ad988185..2251be82a39e 100644
--- a/tests/py/ip6/icmpv6.t.json
+++ b/tests/py/ip6/icmpv6.t.json
@@ -856,6 +856,67 @@
     }
 ]
 
+# icmpv6 id 1
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": {
+                "set": [
+                    "echo-request",
+                    "echo-reply"
+                ]
+            }
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "id",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": 1
+        }
+    }
+]
+
+# icmpv6 type echo-reply icmpv6 id 65534
+[
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "type",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": "echo-reply"
+        }
+    },
+    {
+        "match": {
+            "left": {
+                "payload": {
+                    "field": "id",
+                    "protocol": "icmpv6"
+                }
+            },
+            "op": "==",
+            "right": 65534
+        }
+    }
+]
+
 # icmpv6 sequence 2
 [
     {
diff --git a/tests/py/ip6/icmpv6.t.payload.ip6 b/tests/py/ip6/icmpv6.t.payload.ip6
index 76df184cd0d0..0e96be2d0788 100644
--- a/tests/py/ip6/icmpv6.t.payload.ip6
+++ b/tests/py/ip6/icmpv6.t.payload.ip6
@@ -407,6 +407,27 @@ ip6 test-ip6 input
   [ payload load 2b @ transport header + 4 => reg 1 ]
   [ lookup reg 1 set __set%d 0x1 ]
 
+# icmpv6 id 1
+__set%d test-ip6 3 size 2
+__set%d test-ip6 0
+	element 00000080  : 0 [end]	element 00000081  : 0 [end]
+ip6
+  [ meta load l4proto => reg 1 ]
+  [ cmp eq reg 1 0x0000003a ]
+  [ payload load 1b @ transport header + 0 => reg 1 ]
+  [ lookup reg 1 set __set%d ]
+  [ payload load 2b @ transport header + 4 => reg 1 ]
+  [ cmp eq reg 1 0x00000100 ]
+
+# icmpv6 type echo-reply icmpv6 id 65534
+ip6
+  [ meta load l4proto => reg 1 ]
+  [ cmp eq reg 1 0x0000003a ]
+  [ payload load 1b @ transport header + 0 => reg 1 ]
+  [ cmp eq reg 1 0x00000081 ]
+  [ payload load 2b @ transport header + 4 => reg 1 ]
+  [ cmp eq reg 1 0x0000feff ]
+
 # icmpv6 sequence 2
 __set%d test-ip6 3
 __set%d test-ip6 0
-- 
2.31.1

Re: [PATCH nft v2 1/3] netlink_delinearize: add missing icmp id/sequence support

[Phil Sutter]

Hi,

On Tue, Jun 15, 2021 at 06:01:49PM +0200, Florian Westphal wrote:
> Pablo reports following input and output:
> in: icmpv6 id 1
> out: icmpv6 type { echo-request, echo-reply } icmpv6 parameter-problem 65536/16
> 
> Reason is that icmp fields overlap, decoding of the correct name requires
> check of the icmpv6 type.  This only works for equality tests, for
> instance
> 
> in: icmpv6 type echo-request icmpv6 id 1
> will be listed as "icmpv6 id 1" (which is not correct either, since the
> input only matches on echo-request).
> 
> with this patch, output of 'icmpv6 id 1' is
> icmpv6 type { echo-request, echo-reply } icmpv6 id 1
> 
> The second problem, the removal of a single check (request OR reply),
> is resolved in the followup patch.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

Eric reported a testcase in which this patch seems to cause a segfault
(bisected). The test is as simple as:

| nft -f - <<EOF
| add table inet firewalld_check_rule_index
| add chain inet firewalld_check_rule_index foobar { type filter hook input priority 0 ; }
| add rule inet firewalld_check_rule_index foobar tcp dport 1234 accept
| add rule inet firewalld_check_rule_index foobar accept
| insert rule inet firewalld_check_rule_index foobar index 1 udp dport 4321 accept
| EOF

But a ruleset is in place at this time. Also, I can't reproduce it on my
own machine but only on Eric's VM for testing.

[...]
>  static void payload_match_postprocess(struct rule_pp_ctx *ctx,
>  				      struct expr *expr,
>  				      struct expr *payload)
> @@ -1883,6 +1932,19 @@ static void payload_match_postprocess(struct rule_pp_ctx *ctx,
>  		if (expr->right->etype == EXPR_VALUE) {
>  			payload_match_expand(ctx, expr, payload);
>  			break;
> +		} else if (expr->right->etype == EXPR_SET_REF) {
> +			struct set *set = expr->right->set;
> +
> +			if (set_is_anonymous(set->flags) &&
> +			    !list_empty(&set->init->expressions)) {

According to GDB, set->init is NULL here.

I am not familiar with recent changes in cache code, maybe there's the
actual culprit: Debug printf in cache_init_objects() states flags
variable is 0x4000005f, i.e. NFT_CACHE_SETELEM_BIT is not set.

I am not sure if caching is incomplete and we need that bit or if the
above code should expect sets with missing elements and therefore check
'set->init != NULL' before accessing expressions field.

Cheers, Phil

Re: [PATCH nft v2 1/3] netlink_delinearize: add missing icmp id/sequence support

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> > will be listed as "icmpv6 id 1" (which is not correct either, since the
> > input only matches on echo-request).
> > 
> > with this patch, output of 'icmpv6 id 1' is
> > icmpv6 type { echo-request, echo-reply } icmpv6 id 1
> > 
> > The second problem, the removal of a single check (request OR reply),
> > is resolved in the followup patch.
> > 
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> 
> Eric reported a testcase in which this patch seems to cause a segfault
> (bisected). The test is as simple as:
> 
> | nft -f - <<EOF
> | add table inet firewalld_check_rule_index
> | add chain inet firewalld_check_rule_index foobar { type filter hook input priority 0 ; }
> | add rule inet firewalld_check_rule_index foobar tcp dport 1234 accept
> | add rule inet firewalld_check_rule_index foobar accept
> | insert rule inet firewalld_check_rule_index foobar index 1 udp dport 4321 accept
> | EOF
> 
> But a ruleset is in place at this time. Also, I can't reproduce it on my
> own machine but only on Eric's VM for testing.

You need to add a ruleset with
icmp id 42

then it will crash.  adding 'list ruleset' before EOF avoids it,
because cache gets populated.

> I am not familiar with recent changes in cache code, maybe there's the
> actual culprit: Debug printf in cache_init_objects() states flags
> variable is 0x4000005f, i.e. NFT_CACHE_SETELEM_BIT is not set.
> 
> I am not sure if caching is incomplete and we need that bit or if the
> above code should expect sets with missing elements and therefore check
> 'set->init != NULL' before accessing expressions field.

I think correct fix is to check set->init, we don't need to do
postprocessing if its not going to be printed.

Re: [PATCH nft v2 1/3] netlink_delinearize: add missing icmp id/sequence support

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Eric reported a testcase in which this patch seems to cause a segfault
> (bisected). The test is as simple as:

I've pushed fix and a test case.

Re: [PATCH nft v2 1/3] netlink_delinearize: add missing icmp id/sequence support

[Phil Sutter]

Hey,

On Wed, Jun 30, 2021 at 05:58:26PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Eric reported a testcase in which this patch seems to cause a segfault
> > (bisected). The test is as simple as:
> 
> I've pushed fix and a test case.

Awesome, thanks for the quick patch and test case!

Cheers, Phil