[iptables PATCH 0/3] Follow-up on dangling set fix

[iptables PATCH 0/3] Follow-up on dangling set fix

[Phil Sutter]

While testing/analyzing the changes in commit 4e95200ded923, I noticed
comparison of rules containing among matches was not behaving right. In
fact, most part of the among match data was ignored when comparing, due
to the way among extension scales its payload. This problem exists since
day 1 of the extension implementation for ebtables-nft. Patch 1 fixes
this by placing a hash of the "invisible" data in well-known space.

Patch 2 is a minor cleanup of commit 4e95200ded923, eliminating some
ineffective function signature changes.

Patch 3 adds set (with element) dumps to debug output.

Note about 4e95200ded923 itself: I don't quite like the approach of
conditionally converting a rule into libnftnl format using only compat
expressions for extensions. I am aware my proposed compatibility mode
does the same, but it's a global switch changing add_match() behaviour
consistently. What the commit above does works only because for rule
comparison, both rules are converted back into iptables_command_state
objects. I'd like to follow an alternative path of delaying the
rule conversion so that it does not happen in nft_cmd_new() but later
from nft_action() (or so). This should eliminate some back-and-forth and
also implicitly fix the case of needless set creation.

Phil Sutter (3):
  extensions: libebt_among: Fix for false positive match comparison
  nft: Do not pass nft_rule_ctx to add_nft_among()
  nft: Include sets in debug output

 extensions/libebt_among.c                     |  1 +
 iptables/nft-bridge.h                         | 16 ++++++++
 iptables/nft-cache.c                          | 10 ++++-
 iptables/nft-ruleparse-bridge.c               |  2 +
 iptables/nft.c                                | 17 +++++---
 .../testcases/ebtables/0009-among-lookup_0    | 39 +++++++++++++++++++
 6 files changed, 78 insertions(+), 7 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/ebtables/0009-among-lookup_0

-- 
2.40.0

[iptables PATCH 1/3] extensions: libebt_among: Fix for false positive match comparison

[Phil Sutter]

When comparing matches for equality, trailing data in among match is not
considered. Therefore two matches with identical pairs count may be
treated as identical when the pairs actually differ.

Matches' parsing callbacks have no access to the xtables_match itself,
so can't update userspacesize field as needed.

To fix this, extend struct nft_among_data by a hash field to contain a
DJB hash of the trailing data.

Fixes: 26753888720d8 ("nft: bridge: Rudimental among extension support")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 extensions/libebt_among.c                     |  1 +
 iptables/nft-bridge.h                         | 16 ++++++++
 iptables/nft-ruleparse-bridge.c               |  2 +
 .../testcases/ebtables/0009-among-lookup_0    | 39 +++++++++++++++++++
 4 files changed, 58 insertions(+)
 create mode 100755 iptables/tests/shell/testcases/ebtables/0009-among-lookup_0

diff --git a/extensions/libebt_among.c b/extensions/libebt_among.c
index a80fb80404ee1..c6132f187f5c3 100644
--- a/extensions/libebt_among.c
+++ b/extensions/libebt_among.c
@@ -179,6 +179,7 @@ static int bramong_parse(int c, char **argv, int invert,
 	have_ip = nft_among_pairs_have_ip(optarg);
 	poff = nft_among_prepare_data(data, dst, cnt, invert, have_ip);
 	parse_nft_among_pairs(data->pairs + poff, optarg, cnt, have_ip);
+	nft_among_update_hash(data);
 
 	if (c == AMONG_DST_F || c == AMONG_SRC_F) {
 		munmap(argv, flen);
diff --git a/iptables/nft-bridge.h b/iptables/nft-bridge.h
index eb1b3928b6543..dacdcb58a7895 100644
--- a/iptables/nft-bridge.h
+++ b/iptables/nft-bridge.h
@@ -133,6 +133,7 @@ struct nft_among_data {
 		bool inv;
 		bool ip;
 	} src, dst;
+	uint32_t pairs_hash;
 	/* first source, then dest pairs */
 	struct nft_among_pair pairs[0];
 };
@@ -178,4 +179,19 @@ nft_among_insert_pair(struct nft_among_pair *pairs,
 	(*pcount)++;
 }
 
+/* hash pairs using DJB hash */
+static inline void
+nft_among_update_hash(struct nft_among_data *data)
+{
+	int len = (data->src.cnt + data->dst.cnt) *
+			sizeof(struct nft_among_pair);
+	char *p = (char *)data->pairs;
+	uint32_t hash = 5381;
+
+	while (len > 0)
+		hash = ((hash << 5) + hash) + p[--len];
+
+	data->pairs_hash = hash;
+}
+
 #endif
diff --git a/iptables/nft-ruleparse-bridge.c b/iptables/nft-ruleparse-bridge.c
index 50fb92833046a..4e56d85a318c2 100644
--- a/iptables/nft-ruleparse-bridge.c
+++ b/iptables/nft-ruleparse-bridge.c
@@ -374,6 +374,8 @@ static void nft_bridge_parse_lookup(struct nft_xt_ctx *ctx,
 	if (set_elems_to_among_pairs(among_data->pairs + poff, s, cnt))
 		xtables_error(OTHER_PROBLEM,
 			      "ebtables among pair parsing failed");
+
+	nft_among_update_hash(among_data);
 }
 
 static void parse_watcher(void *object, struct ebt_match **match_list,
diff --git a/iptables/tests/shell/testcases/ebtables/0009-among-lookup_0 b/iptables/tests/shell/testcases/ebtables/0009-among-lookup_0
new file mode 100755
index 0000000000000..c2d2497ad9e12
--- /dev/null
+++ b/iptables/tests/shell/testcases/ebtables/0009-among-lookup_0
@@ -0,0 +1,39 @@
+#!/bin/sh
+
+case "$XT_MULTI" in
+*xtables-nft-multi)
+	;;
+*)
+	echo "skip $XT_MULTI"
+	exit 0
+	;;
+esac
+
+$XT_MULTI ebtables -A FORWARD --among-src fe:ed:ba:be:00:01=10.0.0.1 -j ACCEPT || {
+	echo "sample rule add failed"
+	exit 1
+}
+
+$XT_MULTI ebtables --check FORWARD --among-src fe:ed:ba:be:00:01=10.0.0.1 -j ACCEPT || {
+	echo "--check must find the sample rule"
+	exit 1
+}
+
+$XT_MULTI ebtables --check FORWARD --among-src fe:ed:ba:be:00:01=10.0.0.2 -j ACCEPT && {
+	echo "--check must fail with different payload"
+	exit 1
+}
+$XT_MULTI ebtables --check FORWARD --among-src fe:ed:ba:be:00:01 -j ACCEPT && {
+	echo "--check must fail with shorter payload"
+	exit 1
+}
+$XT_MULTI ebtables --check FORWARD --among-src fe:ed:ba:be:00:01=10.0.0.1,fe:ed:ba:be:00:02=10.0.0.2 -j ACCEPT && {
+	echo "--check must fail with longer payload"
+	exit 1
+}
+$XT_MULTI ebtables -D FORWARD --among-src fe:ed:ba:be:00:01=10.0.0.1 -j ACCEPT || {
+	echo "sample rule deletion failed"
+	exit 1
+}
+
+exit 0
-- 
2.40.0

[iptables PATCH 2/3] nft: Do not pass nft_rule_ctx to add_nft_among()

[Phil Sutter]

It is not used, must be a left-over from an earlier version of the fixed
commit.

Fixes: 4e95200ded923 ("nft-bridge: pass context structure to ops->add() to improve anonymous set support")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 230946d30108d..f453f07acb7e9 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1154,8 +1154,7 @@ gen_lookup(uint32_t sreg, const char *set_name, uint32_t set_id, uint32_t flags)
 #define NFT_DATATYPE_ETHERADDR	9
 
 static int __add_nft_among(struct nft_handle *h, const char *table,
-			   struct nft_rule_ctx *ctx, struct nftnl_rule *r,
-			   struct nft_among_pair *pairs,
+			   struct nftnl_rule *r, struct nft_among_pair *pairs,
 			   int cnt, bool dst, bool inv, bool ip)
 {
 	uint32_t set_id, type = NFT_DATATYPE_ETHERADDR, len = ETH_ALEN;
@@ -1236,7 +1235,7 @@ static int __add_nft_among(struct nft_handle *h, const char *table,
 	return 0;
 }
 
-static int add_nft_among(struct nft_handle *h, struct nft_rule_ctx *ctx,
+static int add_nft_among(struct nft_handle *h,
 			 struct nftnl_rule *r, struct xt_entry_match *m)
 {
 	struct nft_among_data *data = (struct nft_among_data *)m->data;
@@ -1252,10 +1251,10 @@ static int add_nft_among(struct nft_handle *h, struct nft_rule_ctx *ctx,
 	}
 
 	if (data->src.cnt)
-		__add_nft_among(h, table, ctx, r, data->pairs, data->src.cnt,
+		__add_nft_among(h, table, r, data->pairs, data->src.cnt,
 				false, data->src.inv, data->src.ip);
 	if (data->dst.cnt)
-		__add_nft_among(h, table, ctx, r, data->pairs + data->src.cnt,
+		__add_nft_among(h, table, r, data->pairs + data->src.cnt,
 				data->dst.cnt, true, data->dst.inv,
 				data->dst.ip);
 	return 0;
@@ -1476,7 +1475,7 @@ int add_match(struct nft_handle *h, struct nft_rule_ctx *ctx,
 		if (!strcmp(m->u.user.name, "limit"))
 			return add_nft_limit(r, m);
 		else if (!strcmp(m->u.user.name, "among"))
-			return add_nft_among(h, ctx, r, m);
+			return add_nft_among(h, r, m);
 		else if (!strcmp(m->u.user.name, "udp"))
 			return add_nft_udp(h, r, m);
 		else if (!strcmp(m->u.user.name, "tcp"))
-- 
2.40.0

[iptables PATCH 3/3] nft: Include sets in debug output

[Phil Sutter]

Rules referencing them are incomplete without, so add debug output on
the same level as for rules.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cache.c | 10 +++++++++-
 iptables/nft.c       |  6 ++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 76e99adcb8566..fabb577903f28 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -417,6 +417,7 @@ static int set_fetch_elem_cb(struct nftnl_set *s, void *data)
 	char buf[MNL_SOCKET_BUFFER_SIZE];
 	struct nft_handle *h = data;
 	struct nlmsghdr *nlh;
+	int ret;
 
 	if (set_has_elements(s))
 		return 0;
@@ -425,7 +426,14 @@ static int set_fetch_elem_cb(struct nftnl_set *s, void *data)
 				    NLM_F_DUMP, h->seq);
 	nftnl_set_elems_nlmsg_build_payload(nlh, s);
 
-	return mnl_talk(h, nlh, set_elem_cb, s);
+	ret = mnl_talk(h, nlh, set_elem_cb, s);
+
+	if (!ret && h->verbose > 1) {
+		fprintf(stdout, "set ");
+		nftnl_set_fprintf(stdout, s, 0, 0);
+		fprintf(stdout, "\n");
+	}
+	return ret;
 }
 
 static int fetch_set_cache(struct nft_handle *h,
diff --git a/iptables/nft.c b/iptables/nft.c
index f453f07acb7e9..b702c65ae49aa 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2975,6 +2975,12 @@ static void nft_compat_setelem_batch_add(struct nft_handle *h, uint16_t type,
 			break;
 	}
 	nftnl_set_elems_iter_destroy(iter);
+
+	if (h->verbose > 1) {
+		fprintf(stdout, "set ");
+		nftnl_set_fprintf(stdout, set, 0, 0);
+		fprintf(stdout, "\n");
+	}
 }
 
 static void nft_compat_chain_batch_add(struct nft_handle *h, uint16_t type,
-- 
2.40.0

Re: [iptables PATCH 1/3] extensions: libebt_among: Fix for false positive match comparison

[Pablo Neira Ayuso]

Hi Phil,

On Sat, Jul 15, 2023 at 02:59:26PM +0200, Phil Sutter wrote:
> When comparing matches for equality, trailing data in among match is not
> considered. Therefore two matches with identical pairs count may be
> treated as identical when the pairs actually differ.

By "trailing data", you mean the right-hand side of this?

        fe:ed:ba:be:00:01=10.0.0.1

> Matches' parsing callbacks have no access to the xtables_match itself,
> so can't update userspacesize field as needed.
> 
> To fix this, extend struct nft_among_data by a hash field to contain a
> DJB hash of the trailing data.

Is this DJB hash use subject to collisions?

Thanks

> Fixes: 26753888720d8 ("nft: bridge: Rudimental among extension support")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  extensions/libebt_among.c                     |  1 +
>  iptables/nft-bridge.h                         | 16 ++++++++
>  iptables/nft-ruleparse-bridge.c               |  2 +
>  .../testcases/ebtables/0009-among-lookup_0    | 39 +++++++++++++++++++
>  4 files changed, 58 insertions(+)
>  create mode 100755 iptables/tests/shell/testcases/ebtables/0009-among-lookup_0
> 
> diff --git a/extensions/libebt_among.c b/extensions/libebt_among.c
> index a80fb80404ee1..c6132f187f5c3 100644
> --- a/extensions/libebt_among.c
> +++ b/extensions/libebt_among.c
> @@ -179,6 +179,7 @@ static int bramong_parse(int c, char **argv, int invert,
>  	have_ip = nft_among_pairs_have_ip(optarg);
>  	poff = nft_among_prepare_data(data, dst, cnt, invert, have_ip);
>  	parse_nft_among_pairs(data->pairs + poff, optarg, cnt, have_ip);
> +	nft_among_update_hash(data);
>  
>  	if (c == AMONG_DST_F || c == AMONG_SRC_F) {
>  		munmap(argv, flen);
> diff --git a/iptables/nft-bridge.h b/iptables/nft-bridge.h
> index eb1b3928b6543..dacdcb58a7895 100644
> --- a/iptables/nft-bridge.h
> +++ b/iptables/nft-bridge.h
> @@ -133,6 +133,7 @@ struct nft_among_data {
>  		bool inv;
>  		bool ip;
>  	} src, dst;
> +	uint32_t pairs_hash;
>  	/* first source, then dest pairs */
>  	struct nft_among_pair pairs[0];
>  };
> @@ -178,4 +179,19 @@ nft_among_insert_pair(struct nft_among_pair *pairs,
>  	(*pcount)++;
>  }
>  
> +/* hash pairs using DJB hash */
> +static inline void
> +nft_among_update_hash(struct nft_among_data *data)
> +{
> +	int len = (data->src.cnt + data->dst.cnt) *
> +			sizeof(struct nft_among_pair);
> +	char *p = (char *)data->pairs;
> +	uint32_t hash = 5381;
> +
> +	while (len > 0)
> +		hash = ((hash << 5) + hash) + p[--len];
> +
> +	data->pairs_hash = hash;
> +}
> +
>  #endif
> diff --git a/iptables/nft-ruleparse-bridge.c b/iptables/nft-ruleparse-bridge.c
> index 50fb92833046a..4e56d85a318c2 100644
> --- a/iptables/nft-ruleparse-bridge.c
> +++ b/iptables/nft-ruleparse-bridge.c
> @@ -374,6 +374,8 @@ static void nft_bridge_parse_lookup(struct nft_xt_ctx *ctx,
>  	if (set_elems_to_among_pairs(among_data->pairs + poff, s, cnt))
>  		xtables_error(OTHER_PROBLEM,
>  			      "ebtables among pair parsing failed");
> +
> +	nft_among_update_hash(among_data);
>  }
>  
>  static void parse_watcher(void *object, struct ebt_match **match_list,
> diff --git a/iptables/tests/shell/testcases/ebtables/0009-among-lookup_0 b/iptables/tests/shell/testcases/ebtables/0009-among-lookup_0
> new file mode 100755
> index 0000000000000..c2d2497ad9e12
> --- /dev/null
> +++ b/iptables/tests/shell/testcases/ebtables/0009-among-lookup_0
> @@ -0,0 +1,39 @@
> +#!/bin/sh
> +
> +case "$XT_MULTI" in
> +*xtables-nft-multi)
> +	;;
> +*)
> +	echo "skip $XT_MULTI"
> +	exit 0
> +	;;
> +esac
> +
> +$XT_MULTI ebtables -A FORWARD --among-src fe:ed:ba:be:00:01=10.0.0.1 -j ACCEPT || {
> +	echo "sample rule add failed"
> +	exit 1
> +}
> +
> +$XT_MULTI ebtables --check FORWARD --among-src fe:ed:ba:be:00:01=10.0.0.1 -j ACCEPT || {
> +	echo "--check must find the sample rule"
> +	exit 1
> +}
> +
> +$XT_MULTI ebtables --check FORWARD --among-src fe:ed:ba:be:00:01=10.0.0.2 -j ACCEPT && {
> +	echo "--check must fail with different payload"
> +	exit 1
> +}
> +$XT_MULTI ebtables --check FORWARD --among-src fe:ed:ba:be:00:01 -j ACCEPT && {
> +	echo "--check must fail with shorter payload"
> +	exit 1
> +}
> +$XT_MULTI ebtables --check FORWARD --among-src fe:ed:ba:be:00:01=10.0.0.1,fe:ed:ba:be:00:02=10.0.0.2 -j ACCEPT && {
> +	echo "--check must fail with longer payload"
> +	exit 1
> +}
> +$XT_MULTI ebtables -D FORWARD --among-src fe:ed:ba:be:00:01=10.0.0.1 -j ACCEPT || {
> +	echo "sample rule deletion failed"
> +	exit 1
> +}
> +
> +exit 0
> -- 
> 2.40.0
> 

Re: [iptables PATCH 1/3] extensions: libebt_among: Fix for false positive match comparison

[Phil Sutter]

On Mon, Jul 17, 2023 at 01:07:35PM +0200, Pablo Neira Ayuso wrote:
> Hi Phil,
> 
> On Sat, Jul 15, 2023 at 02:59:26PM +0200, Phil Sutter wrote:
> > When comparing matches for equality, trailing data in among match is not
> > considered. Therefore two matches with identical pairs count may be
> > treated as identical when the pairs actually differ.
> 
> By "trailing data", you mean the right-hand side of this?
> 
>         fe:ed:ba:be:00:01=10.0.0.1

I mean field "pairs" in struct nft_among_data:

| struct nft_among_data {
|         struct {
|                 size_t cnt;
|                 bool inv;
|                 bool ip;
|         } src, dst; 
|         uint32_t pairs_hash;
|         /* first source, then dest pairs */
|         struct nft_among_pair pairs[0];
| };

So basically all pairs are being ignored. As long as the number and type (with
IP or not) of pairs and the invert flag matches, match data was considered
identical.

> > Matches' parsing callbacks have no access to the xtables_match itself,
> > so can't update userspacesize field as needed.
> > 
> > To fix this, extend struct nft_among_data by a hash field to contain a
> > DJB hash of the trailing data.
> 
> Is this DJB hash use subject to collisions?

Could be. I considered it "good enough", but didn't try how easy it is
to cause a collision. Are you suggesting to use a more secure algorithm?

Thanks, Phil

Re: [iptables PATCH 1/3] extensions: libebt_among: Fix for false positive match comparison

[Phil Sutter]

Pablo,

On Mon, Jul 17, 2023 at 01:07:35PM +0200, Pablo Neira Ayuso wrote:
> On Sat, Jul 15, 2023 at 02:59:26PM +0200, Phil Sutter wrote:
> > When comparing matches for equality, trailing data in among match is not
> > considered. Therefore two matches with identical pairs count may be
> > treated as identical when the pairs actually differ.
> 
> By "trailing data", you mean the right-hand side of this?
> 
>         fe:ed:ba:be:00:01=10.0.0.1
> 
> > Matches' parsing callbacks have no access to the xtables_match itself,
> > so can't update userspacesize field as needed.
> > 
> > To fix this, extend struct nft_among_data by a hash field to contain a
> > DJB hash of the trailing data.
> 
> Is this DJB hash use subject to collisions?

Thanks for the heads-up. I suspected DJB hash algo might not be perfect
when it comes to collisions, but "good enough" for the task. In fact,
collisions are pretty common, so this approach is not a proper solution
to the problem.

Searching for other ways to fix the issue, I noticed that
compare_matches() was deliberately changed to compare only the first
'userspacesize' bytes of extensions to avoid false-negatives caused by
kernel-internals in extension data.

I see two different solutions and would like to hear your opinion. First
one is a hack, special treatment for among match in compare_matches():

| @@ -381,6 +381,7 @@ bool compare_matches(struct xtables_rule_match *mt1,
|         for (mp1 = mt1, mp2 = mt2; mp1 && mp2; mp1 = mp1->next, mp2 = mp2->next) {
|                 struct xt_entry_match *m1 = mp1->match->m;
|                 struct xt_entry_match *m2 = mp2->match->m;
| +               size_t cmplen = mp1->match->userspacesize;
|  
|                 if (strcmp(m1->u.user.name, m2->u.user.name) != 0) {
|                         DEBUGP("mismatching match name\n");
| @@ -392,8 +393,10 @@ bool compare_matches(struct xtables_rule_match *mt1,
|                         return false;
|                 }
|  
| -               if (memcmp(m1->data, m2->data,
| -                          mp1->match->userspacesize) != 0) {
| +               if (!strcmp(m1->u.user.name, "among"))
| +                       cmplen = m1->u.match_size - sizeof(*m1);
| +
| +               if (memcmp(m1->data, m2->data, cmplen) != 0) {
|                         DEBUGP("mismatch match data\n");
|                         return false;
|                 }

The second one is more generic, reusing extensions' 'udata' pointer. One
could make xtables_option_{m,t}fcall() functions zero the scratch area
if present (so locally created extensions match ones fetched from
kernel) and compare that scratch area in compare_matches(). For among
match, using the scratch area to store pairs is fine.

Cheers, Phil

Re: [iptables PATCH 1/3] extensions: libebt_among: Fix for false positive match comparison

[Pablo Neira Ayuso]

Hi Phil,

On Fri, Jul 21, 2023 at 11:59:55AM +0200, Phil Sutter wrote:
> Pablo,
> 
> On Mon, Jul 17, 2023 at 01:07:35PM +0200, Pablo Neira Ayuso wrote:
> > On Sat, Jul 15, 2023 at 02:59:26PM +0200, Phil Sutter wrote:
> > > When comparing matches for equality, trailing data in among match is not
> > > considered. Therefore two matches with identical pairs count may be
> > > treated as identical when the pairs actually differ.
> > 
> > By "trailing data", you mean the right-hand side of this?
> > 
> >         fe:ed:ba:be:00:01=10.0.0.1
> > 
> > > Matches' parsing callbacks have no access to the xtables_match itself,
> > > so can't update userspacesize field as needed.
> > > 
> > > To fix this, extend struct nft_among_data by a hash field to contain a
> > > DJB hash of the trailing data.
> > 
> > Is this DJB hash use subject to collisions?
> 
> Thanks for the heads-up. I suspected DJB hash algo might not be perfect
> when it comes to collisions, but "good enough" for the task. In fact,
> collisions are pretty common, so this approach is not a proper solution
> to the problem.
>
> Searching for other ways to fix the issue, I noticed that
> compare_matches() was deliberately changed to compare only the first
> 'userspacesize' bytes of extensions to avoid false-negatives caused by
> kernel-internals in extension data.

Indeed, that was a deliberate decision.

> I see two different solutions and would like to hear your opinion. First
> one is a hack, special treatment for among match in compare_matches():
> 
> | @@ -381,6 +381,7 @@ bool compare_matches(struct xtables_rule_match *mt1,
> |         for (mp1 = mt1, mp2 = mt2; mp1 && mp2; mp1 = mp1->next, mp2 = mp2->next) {
> |                 struct xt_entry_match *m1 = mp1->match->m;
> |                 struct xt_entry_match *m2 = mp2->match->m;
> | +               size_t cmplen = mp1->match->userspacesize;
> |  
> |                 if (strcmp(m1->u.user.name, m2->u.user.name) != 0) {
> |                         DEBUGP("mismatching match name\n");
> | @@ -392,8 +393,10 @@ bool compare_matches(struct xtables_rule_match *mt1,
> |                         return false;
> |                 }
> |  
> | -               if (memcmp(m1->data, m2->data,
> | -                          mp1->match->userspacesize) != 0) {
> | +               if (!strcmp(m1->u.user.name, "among"))
> | +                       cmplen = m1->u.match_size - sizeof(*m1);
> | +
> | +               if (memcmp(m1->data, m2->data, cmplen) != 0) {
> |                         DEBUGP("mismatch match data\n");
> |                         return false;
> |                 }

This incremental update is relatively simple and it is only 'among'
that requires this special handling. Maybe you start with this, also
placing a comment to describe the intention for this particular case.
I don't remember if among allows to delete a rule with set elements
that are placed in different order.

Then, if you have to follow up because this is not enough...

> The second one is more generic, reusing extensions' 'udata' pointer. One
> could make xtables_option_{m,t}fcall() functions zero the scratch area
> if present (so locally created extensions match ones fetched from
> kernel) and compare that scratch area in compare_matches(). For among
> match, using the scratch area to store pairs is fine.

then pursue this second approach?

Thanks for explaining.

Re: [iptables PATCH 1/3] extensions: libebt_among: Fix for false positive match comparison

[Phil Sutter]

On Fri, Jul 21, 2023 at 03:56:13PM +0200, Pablo Neira Ayuso wrote:
> Hi Phil,
> 
> On Fri, Jul 21, 2023 at 11:59:55AM +0200, Phil Sutter wrote:
> > Pablo,
> > 
> > On Mon, Jul 17, 2023 at 01:07:35PM +0200, Pablo Neira Ayuso wrote:
> > > On Sat, Jul 15, 2023 at 02:59:26PM +0200, Phil Sutter wrote:
> > > > When comparing matches for equality, trailing data in among match is not
> > > > considered. Therefore two matches with identical pairs count may be
> > > > treated as identical when the pairs actually differ.
> > > 
> > > By "trailing data", you mean the right-hand side of this?
> > > 
> > >         fe:ed:ba:be:00:01=10.0.0.1
> > > 
> > > > Matches' parsing callbacks have no access to the xtables_match itself,
> > > > so can't update userspacesize field as needed.
> > > > 
> > > > To fix this, extend struct nft_among_data by a hash field to contain a
> > > > DJB hash of the trailing data.
> > > 
> > > Is this DJB hash use subject to collisions?
> > 
> > Thanks for the heads-up. I suspected DJB hash algo might not be perfect
> > when it comes to collisions, but "good enough" for the task. In fact,
> > collisions are pretty common, so this approach is not a proper solution
> > to the problem.
> >
> > Searching for other ways to fix the issue, I noticed that
> > compare_matches() was deliberately changed to compare only the first
> > 'userspacesize' bytes of extensions to avoid false-negatives caused by
> > kernel-internals in extension data.
> 
> Indeed, that was a deliberate decision.

Yes, you did it! :)

> > I see two different solutions and would like to hear your opinion. First
> > one is a hack, special treatment for among match in compare_matches():
> > 
> > | @@ -381,6 +381,7 @@ bool compare_matches(struct xtables_rule_match *mt1,
> > |         for (mp1 = mt1, mp2 = mt2; mp1 && mp2; mp1 = mp1->next, mp2 = mp2->next) {
> > |                 struct xt_entry_match *m1 = mp1->match->m;
> > |                 struct xt_entry_match *m2 = mp2->match->m;
> > | +               size_t cmplen = mp1->match->userspacesize;
> > |  
> > |                 if (strcmp(m1->u.user.name, m2->u.user.name) != 0) {
> > |                         DEBUGP("mismatching match name\n");
> > | @@ -392,8 +393,10 @@ bool compare_matches(struct xtables_rule_match *mt1,
> > |                         return false;
> > |                 }
> > |  
> > | -               if (memcmp(m1->data, m2->data,
> > | -                          mp1->match->userspacesize) != 0) {
> > | +               if (!strcmp(m1->u.user.name, "among"))
> > | +                       cmplen = m1->u.match_size - sizeof(*m1);
> > | +
> > | +               if (memcmp(m1->data, m2->data, cmplen) != 0) {
> > |                         DEBUGP("mismatch match data\n");
> > |                         return false;
> > |                 }
> 
> This incremental update is relatively simple and it is only 'among'
> that requires this special handling. Maybe you start with this, also
> placing a comment to describe the intention for this particular case.
> I don't remember if among allows to delete a rule with set elements
> that are placed in different order.
> 
> Then, if you have to follow up because this is not enough...

Luckily, I had that in mind already and implemented element sorting in
the among parser, it should match how the kernel returns the elements.

> > The second one is more generic, reusing extensions' 'udata' pointer. One
> > could make xtables_option_{m,t}fcall() functions zero the scratch area
> > if present (so locally created extensions match ones fetched from
> > kernel) and compare that scratch area in compare_matches(). For among
> > match, using the scratch area to store pairs is fine.
> 
> then pursue this second approach?

ACK, I'll keep that around somewhere. For now that special casing above
is probably fine.

Thanks, Phil

Re: [iptables PATCH 0/3] Follow-up on dangling set fix

[Phil Sutter]

On Sat, Jul 15, 2023 at 02:59:25PM +0200, Phil Sutter wrote:
> While testing/analyzing the changes in commit 4e95200ded923, I noticed
> comparison of rules containing among matches was not behaving right. In
> fact, most part of the among match data was ignored when comparing, due
> to the way among extension scales its payload. This problem exists since
> day 1 of the extension implementation for ebtables-nft. Patch 1 fixes
> this by placing a hash of the "invisible" data in well-known space.
> 
> Patch 2 is a minor cleanup of commit 4e95200ded923, eliminating some
> ineffective function signature changes.
> 
> Patch 3 adds set (with element) dumps to debug output.
> 
> Note about 4e95200ded923 itself: I don't quite like the approach of
> conditionally converting a rule into libnftnl format using only compat
> expressions for extensions. I am aware my proposed compatibility mode
> does the same, but it's a global switch changing add_match() behaviour
> consistently. What the commit above does works only because for rule
> comparison, both rules are converted back into iptables_command_state
> objects. I'd like to follow an alternative path of delaying the
> rule conversion so that it does not happen in nft_cmd_new() but later
> from nft_action() (or so). This should eliminate some back-and-forth and
> also implicitly fix the case of needless set creation.
> 
> Phil Sutter (3):
>   extensions: libebt_among: Fix for false positive match comparison
>   nft: Do not pass nft_rule_ctx to add_nft_among()
>   nft: Include sets in debug output

Applied the last two patches of this series. Patch 1 turned out to be
ineffective (due to frequent collisions). A proper solution is contained
in commit 10583537004f7 ("nft: Special casing for among match in
compare_matches()").