[iptables PATCH 1/3] ebtables: Clone extensions before modifying them

[iptables PATCH 1/3] ebtables: Clone extensions before modifying them

[Phil Sutter]

Upon identifying an extension option, ebt_command_default() would have
the extension parse the option prior to creating a copy for attaching to
the iptables_command_state object. After copying, the (modified)
initial extension's data was cleared.

This somewhat awkward process breaks with among match which increases
match_size if needed (but never reduces it). This change is not undone,
hence leaks into following instances. This in turn is problematic with
ebtables-restore only (as multiple rules are parsed) and specifically
when deleting rules as the potentially over-sized match_size won't match
the one parsed from the kernel.

A workaround would be to make bramong_parse() realloc the match also if
new size is smaller than the old one. This patch attempts a proper fix
though, by making ebt_command_default() copy the extension first and
parsing the option into the copy afterwards.

No Fixes tag: Prior to commit 24bb57d3f52ac ("ebtables: Support for
guided option parser"), ebtables relied upon the extension's parser
return code instead of checking option_offset, so copying the extension
opportunistically wasn't feasible.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-bridge.h |  8 ++++----
 iptables/xtables-eb.c | 16 ++++++++++------
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/iptables/nft-bridge.h b/iptables/nft-bridge.h
index 13b077fc4fbf3..54b473ebff6b0 100644
--- a/iptables/nft-bridge.h
+++ b/iptables/nft-bridge.h
@@ -108,10 +108,10 @@ static inline const char *ebt_target_name(unsigned int verdict)
 })								\
 
 void ebt_cs_clean(struct iptables_command_state *cs);
-void ebt_add_match(struct xtables_match *m,
-			  struct iptables_command_state *cs);
-void ebt_add_watcher(struct xtables_target *watcher,
-                     struct iptables_command_state *cs);
+struct xtables_match *ebt_add_match(struct xtables_match *m,
+				    struct iptables_command_state *cs);
+struct xtables_target *ebt_add_watcher(struct xtables_target *watcher,
+				       struct iptables_command_state *cs);
 int ebt_command_default(struct iptables_command_state *cs,
 			struct xtables_globals *unused, bool ebt_invert);
 
diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index 658cf4b98c04d..06386cd90830c 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -367,8 +367,8 @@ static void ebt_load_match_extensions(void)
 	ebt_load_watcher("nflog");
 }
 
-void ebt_add_match(struct xtables_match *m,
-		   struct iptables_command_state *cs)
+struct xtables_match *ebt_add_match(struct xtables_match *m,
+				    struct iptables_command_state *cs)
 {
 	struct xtables_rule_match **rule_matches = &cs->matches;
 	struct xtables_match *newm;
@@ -397,10 +397,12 @@ void ebt_add_match(struct xtables_match *m,
 	for (matchp = &cs->match_list; *matchp; matchp = &(*matchp)->next)
 		;
 	*matchp = newnode;
+
+	return newm;
 }
 
-void ebt_add_watcher(struct xtables_target *watcher,
-		     struct iptables_command_state *cs)
+struct xtables_target *ebt_add_watcher(struct xtables_target *watcher,
+				       struct iptables_command_state *cs)
 {
 	struct ebt_match *newnode, **matchp;
 	struct xtables_target *clone;
@@ -425,6 +427,8 @@ void ebt_add_watcher(struct xtables_target *watcher,
 	for (matchp = &cs->match_list; *matchp; matchp = &(*matchp)->next)
 		;
 	*matchp = newnode;
+
+	return clone;
 }
 
 int ebt_command_default(struct iptables_command_state *cs,
@@ -476,8 +480,8 @@ int ebt_command_default(struct iptables_command_state *cs,
 		if (cs->c < m->option_offset ||
 		    cs->c >= m->option_offset + XT_OPTION_OFFSET_SCALE)
 			continue;
+		m = ebt_add_match(m, cs);
 		xtables_option_mpcall(cs->c, cs->argv, ebt_invert, m, &cs->eb);
-		ebt_add_match(m, cs);
 		return 0;
 	}
 
@@ -491,8 +495,8 @@ int ebt_command_default(struct iptables_command_state *cs,
 		if (cs->c < t->option_offset ||
 		    cs->c >= t->option_offset + XT_OPTION_OFFSET_SCALE)
 			continue;
+		t = ebt_add_watcher(t, cs);
 		xtables_option_tpcall(cs->c, cs->argv, ebt_invert, t, &cs->eb);
-		ebt_add_watcher(t, cs);
 		return 0;
 	}
 	if (cs->c == ':')
-- 
2.47.0

[iptables PATCH 2/3] ebtables: Simplify ebt_add_{match,watcher}

[Phil Sutter]

Now that extension options are parsed after these functions return, no
modifications need to be carried over to the clone and undone in the
original.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xtables-eb.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index 06386cd90830c..86c33b4e7dcb6 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -371,23 +371,17 @@ struct xtables_match *ebt_add_match(struct xtables_match *m,
 				    struct iptables_command_state *cs)
 {
 	struct xtables_rule_match **rule_matches = &cs->matches;
-	struct xtables_match *newm;
 	struct ebt_match *newnode, **matchp;
-	struct xt_entry_match *m2;
+	struct xtables_match *newm;
 
 	newm = xtables_find_match(m->name, XTF_LOAD_MUST_SUCCEED, rule_matches);
 	if (newm == NULL)
 		xtables_error(OTHER_PROBLEM,
 			      "Unable to add match %s", m->name);
 
-	m2 = xtables_calloc(1, newm->m->u.match_size);
-	memcpy(m2, newm->m, newm->m->u.match_size);
-	memset(newm->m->data, 0, newm->size);
+	newm->m = xtables_calloc(1, m->m->u.match_size);
+	memcpy(newm->m, m->m, m->m->u.match_size);
 	xs_init_match(newm);
-	newm->m = m2;
-
-	newm->mflags = m->mflags;
-	m->mflags = 0;
 
 	/* glue code for watchers */
 	newnode = xtables_calloc(1, sizeof(struct ebt_match));
@@ -409,17 +403,13 @@ struct xtables_target *ebt_add_watcher(struct xtables_target *watcher,
 
 	clone = xtables_malloc(sizeof(struct xtables_target));
 	memcpy(clone, watcher, sizeof(struct xtables_target));
-	clone->udata = NULL;
-	clone->tflags = watcher->tflags;
 	clone->next = clone;
+	clone->udata = NULL;
+	xs_init_target(clone);
 
 	clone->t = xtables_calloc(1, watcher->t->u.target_size);
 	memcpy(clone->t, watcher->t, watcher->t->u.target_size);
 
-	memset(watcher->t->data, 0, watcher->size);
-	xs_init_target(watcher);
-	watcher->tflags = 0;
-
 
 	newnode = xtables_calloc(1, sizeof(struct ebt_match));
 	newnode->u.watcher = clone;
-- 
2.47.0

[iptables PATCH 3/3] tests: shell: Test ebtables-restore deleting among matches

[Phil Sutter]

Rules containing among match would spuriously fail to compare if there
was a previous rule with larger among match payload.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 .../ebtables/0012-restore-delete-among_0       | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100755 iptables/tests/shell/testcases/ebtables/0012-restore-delete-among_0

diff --git a/iptables/tests/shell/testcases/ebtables/0012-restore-delete-among_0 b/iptables/tests/shell/testcases/ebtables/0012-restore-delete-among_0
new file mode 100755
index 0000000000000..165745e169f89
--- /dev/null
+++ b/iptables/tests/shell/testcases/ebtables/0012-restore-delete-among_0
@@ -0,0 +1,18 @@
+#!/bin/bash -e
+
+case "$XT_MULTI" in
+*xtables-nft-multi)
+	;;
+*)
+	echo "skip $XT_MULTI"
+	exit 0
+	;;
+esac
+
+RULESET='*filter
+-A FORWARD --among-dst de:ad:0:be:ee:ff,c0:ff:ee:0:ba:be
+-A FORWARD --among-dst de:ad:0:be:ee:ff'
+
+$XT_MULTI ebtables-restore <<< "$RULESET"
+echo "$RULESET" | sed -e 's/-A/-D/' | $XT_MULTI ebtables-restore --noflush
+
-- 
2.47.0

Re: [iptables PATCH 1/3] ebtables: Clone extensions before modifying them

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Upon identifying an extension option, ebt_command_default() would have
> the extension parse the option prior to creating a copy for attaching to
> the iptables_command_state object. After copying, the (modified)
> initial extension's data was cleared.

Patches look good to me, thanks for adding test cases.

Acked-by: Florian Westphal <fw@strlen.de>

Re: [iptables PATCH 1/3] ebtables: Clone extensions before modifying them

[Phil Sutter]

On Tue, Nov 05, 2024 at 09:35:41PM +0100, Phil Sutter wrote:
> Upon identifying an extension option, ebt_command_default() would have
> the extension parse the option prior to creating a copy for attaching to
> the iptables_command_state object. After copying, the (modified)
> initial extension's data was cleared.
> 
> This somewhat awkward process breaks with among match which increases
> match_size if needed (but never reduces it). This change is not undone,
> hence leaks into following instances. This in turn is problematic with
> ebtables-restore only (as multiple rules are parsed) and specifically
> when deleting rules as the potentially over-sized match_size won't match
> the one parsed from the kernel.
> 
> A workaround would be to make bramong_parse() realloc the match also if
> new size is smaller than the old one. This patch attempts a proper fix
> though, by making ebt_command_default() copy the extension first and
> parsing the option into the copy afterwards.
> 
> No Fixes tag: Prior to commit 24bb57d3f52ac ("ebtables: Support for
> guided option parser"), ebtables relied upon the extension's parser
> return code instead of checking option_offset, so copying the extension
> opportunistically wasn't feasible.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Series applied.