diff mbox series

[iptables,3/3] xtables: Fix for inserting rule at wrong position

Message ID 20190115222305.2576-4-phil@nwl.cc
State Accepted
Delegated to: Pablo Neira
Headers show
Series xtables: Fix for inserting rule at wrong position | expand

Commit Message

Phil Sutter Jan. 15, 2019, 10:23 p.m. UTC 
iptables-restore allows to insert rules at a certain position which is
problematic for iptables-nft to realize since rule position is not
determined by number but handle of previous or following rule and in
case the rules surrounding the new one are new as well, they don't have
a handle to refer to yet.

Fix this by making use of NFTNL_RULE_POSITION_ID attribute: When
inserting before a rule which does not have a handle, refer to it using
its NFTNL_RULE_ID value. If the latter doesn't exist either, assign a
new one to it.

The last used rule ID value is tracked in a new field of struct
nft_handle which is incremented before each use.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c                                |  30 +++--
 iptables/nft.h                                |   1 +
 .../ipt-restore/0003-restore-ordering_0       | 117 ++++++++++++++++++
 .../testcases/iptables/0005-rule-replace_0    |  38 ++++++
 4 files changed, 176 insertions(+), 10 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0
 create mode 100755 iptables/tests/shell/testcases/iptables/0005-rule-replace_0

Comments

Pablo Neira Ayuso Jan. 28, 2019, 10:29 a.m. UTC | #1 
On Tue, Jan 15, 2019 at 11:23:05PM +0100, Phil Sutter wrote:
> iptables-restore allows to insert rules at a certain position which is
> problematic for iptables-nft to realize since rule position is not
> determined by number but handle of previous or following rule and in
> case the rules surrounding the new one are new as well, they don't have
> a handle to refer to yet.
> 
> Fix this by making use of NFTNL_RULE_POSITION_ID attribute: When
> inserting before a rule which does not have a handle, refer to it using
> its NFTNL_RULE_ID value. If the latter doesn't exist either, assign a
> new one to it.
> 
> The last used rule ID value is tracked in a new field of struct
> nft_handle which is incremented before each use.

Much nicer, thanks a lot for reworking this, Phil.

Can you see any more problems with rule insertions at positions via
iptables-restore?

I guess we'll need a patch for nft too, will you give it a shot?

Thanks.
Phil Sutter Jan. 28, 2019, 12:54 p.m. UTC | #2 
On Mon, Jan 28, 2019 at 11:29:09AM +0100, Pablo Neira Ayuso wrote:
> On Tue, Jan 15, 2019 at 11:23:05PM +0100, Phil Sutter wrote:
> > iptables-restore allows to insert rules at a certain position which is
> > problematic for iptables-nft to realize since rule position is not
> > determined by number but handle of previous or following rule and in
> > case the rules surrounding the new one are new as well, they don't have
> > a handle to refer to yet.
> > 
> > Fix this by making use of NFTNL_RULE_POSITION_ID attribute: When
> > inserting before a rule which does not have a handle, refer to it using
> > its NFTNL_RULE_ID value. If the latter doesn't exist either, assign a
> > new one to it.
> > 
> > The last used rule ID value is tracked in a new field of struct
> > nft_handle which is incremented before each use.
> 
> Much nicer, thanks a lot for reworking this, Phil.

Yes, your RULE_POSITION suggestion was priceless, thanks a lot!

> Can you see any more problems with rule insertions at positions via
> iptables-restore?

I can't think of any, but that doesn't necessarily mean there are none.
:)

> I guess we'll need a patch for nft too, will you give it a shot?

I guess it is possible to create the same problems with 'nft -f' and
separate 'add rule' calls containing 'index' reference. Though according
to nft.8, 'index' has to be that of an existing rule. So in that sense
it's rather a missing feature and I would prefer to treat it as such.

Cheers, Phil
diff mbox series

Patch

diff --git a/iptables/nft.c b/iptables/nft.c
index ee3d142b25247..83d373c95ce9e 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2063,16 +2063,30 @@  int nft_rule_delete(struct nft_handle *h, const char *chain,
 static struct nftnl_rule *
 nft_rule_add(struct nft_handle *h, const char *chain,
 	     const char *table, struct iptables_command_state *cs,
-	     uint64_t handle, bool verbose)
+	     struct nftnl_rule *ref, bool verbose)
 {
 	struct nftnl_rule *r;
+	uint64_t ref_id;
 
 	r = nft_rule_new(h, chain, table, cs);
 	if (r == NULL)
 		return NULL;
 
-	if (handle > 0)
-		nftnl_rule_set_u64(r, NFTNL_RULE_POSITION, handle);
+	if (ref) {
+		ref_id = nftnl_rule_get_u64(ref, NFTNL_RULE_HANDLE);
+		if (ref_id > 0) {
+			nftnl_rule_set_u64(r, NFTNL_RULE_POSITION, ref_id);
+			DEBUGP("adding after rule handle %"PRIu64"\n", ref_id);
+		} else {
+			ref_id = nftnl_rule_get_u32(ref, NFTNL_RULE_ID);
+			if (!ref_id) {
+				ref_id = ++h->rule_id;
+				nftnl_rule_set_u32(ref, NFTNL_RULE_ID, ref_id);
+			}
+			nftnl_rule_set_u32(r, NFTNL_RULE_POSITION_ID, ref_id);
+			DEBUGP("adding after rule ID %"PRIu64"\n", ref_id);
+		}
+	}
 
 	if (batch_rule_add(h, NFT_COMPAT_RULE_INSERT, r) < 0) {
 		nftnl_rule_free(r);
@@ -2088,9 +2102,8 @@  nft_rule_add(struct nft_handle *h, const char *chain,
 int nft_rule_insert(struct nft_handle *h, const char *chain,
 		    const char *table, void *data, int rulenum, bool verbose)
 {
-	struct nftnl_rule *r, *new_rule;
+	struct nftnl_rule *r = NULL, *new_rule;
 	struct nftnl_chain *c;
-	uint64_t handle = 0;
 
 	/* If built-in chains don't exist for this table, create them */
 	if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
@@ -2118,16 +2131,13 @@  int nft_rule_insert(struct nft_handle *h, const char *chain,
 			errno = ENOENT;
 			goto err;
 		}
-
-		handle = nftnl_rule_get_u64(r, NFTNL_RULE_HANDLE);
-		DEBUGP("adding after rule handle %"PRIu64"\n", handle);
 	}
 
-	new_rule = nft_rule_add(h, chain, table, data, handle, verbose);
+	new_rule = nft_rule_add(h, chain, table, data, r, verbose);
 	if (!new_rule)
 		goto err;
 
-	if (handle)
+	if (r)
 		nftnl_chain_rule_insert_at(new_rule, r);
 	else
 		nftnl_chain_rule_add(new_rule, c);
diff --git a/iptables/nft.h b/iptables/nft.h
index 97d73c8b534be..0726923a63dd4 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -32,6 +32,7 @@  struct nft_handle {
 	struct mnl_socket	*nl;
 	uint32_t		portid;
 	uint32_t		seq;
+	uint32_t		rule_id;
 	struct list_head	obj_list;
 	int			obj_list_num;
 	struct nftnl_batch	*batch;
diff --git a/iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0 b/iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0
new file mode 100755
index 0000000000000..51f2422e15259
--- /dev/null
+++ b/iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0
@@ -0,0 +1,117 @@ 
+#!/bin/bash
+
+# Make sure iptables-restore does the right thing
+# when encountering INSERT rules with index.
+
+set -e
+
+# show rules, drop uninteresting policy settings
+ipt_show() {
+	$XT_MULTI iptables -S | grep -v '^-P'
+}
+
+# basic issue reproducer
+
+$XT_MULTI iptables-restore <<EOF
+*filter
+-A FORWARD -m comment --comment "appended rule" -j ACCEPT
+-I FORWARD 1 -m comment --comment "rule 1" -j ACCEPT
+-I FORWARD 2 -m comment --comment "rule 2" -j ACCEPT
+-I FORWARD 3 -m comment --comment "rule 3" -j ACCEPT
+COMMIT
+EOF
+
+EXPECT='-A FORWARD -m comment --comment "rule 1" -j ACCEPT
+-A FORWARD -m comment --comment "rule 2" -j ACCEPT
+-A FORWARD -m comment --comment "rule 3" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule" -j ACCEPT'
+
+diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
+
+# insert rules into existing ruleset
+
+$XT_MULTI iptables-restore --noflush <<EOF
+*filter
+-I FORWARD 1 -m comment --comment "rule 0.5" -j ACCEPT
+-I FORWARD 3 -m comment --comment "rule 1.5" -j ACCEPT
+-I FORWARD 5 -m comment --comment "rule 2.5" -j ACCEPT
+-I FORWARD 7 -m comment --comment "rule 3.5" -j ACCEPT
+-I FORWARD 9 -m comment --comment "appended rule 2" -j ACCEPT
+COMMIT
+EOF
+
+EXPECT='-A FORWARD -m comment --comment "rule 0.5" -j ACCEPT
+-A FORWARD -m comment --comment "rule 1" -j ACCEPT
+-A FORWARD -m comment --comment "rule 1.5" -j ACCEPT
+-A FORWARD -m comment --comment "rule 2" -j ACCEPT
+-A FORWARD -m comment --comment "rule 2.5" -j ACCEPT
+-A FORWARD -m comment --comment "rule 3" -j ACCEPT
+-A FORWARD -m comment --comment "rule 3.5" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule 2" -j ACCEPT'
+
+diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
+
+# insert rules in between added ones
+
+$XT_MULTI iptables-restore <<EOF
+*filter
+-A FORWARD -m comment --comment "appended rule 1" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule 2" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule 3" -j ACCEPT
+-I FORWARD 1 -m comment --comment "rule 1" -j ACCEPT
+-I FORWARD 3 -m comment --comment "rule 2" -j ACCEPT
+-I FORWARD 5 -m comment --comment "rule 3" -j ACCEPT
+COMMIT
+EOF
+
+EXPECT='-A FORWARD -m comment --comment "rule 1" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule 1" -j ACCEPT
+-A FORWARD -m comment --comment "rule 2" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule 2" -j ACCEPT
+-A FORWARD -m comment --comment "rule 3" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule 3" -j ACCEPT'
+
+diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
+
+# test rule deletion in dump files
+
+$XT_MULTI iptables-restore --noflush <<EOF
+*filter
+-D FORWARD -m comment --comment "appended rule 1" -j ACCEPT
+-D FORWARD 3
+-I FORWARD 3 -m comment --comment "manually replaced rule 2" -j ACCEPT
+COMMIT
+EOF
+
+EXPECT='-A FORWARD -m comment --comment "rule 1" -j ACCEPT
+-A FORWARD -m comment --comment "rule 2" -j ACCEPT
+-A FORWARD -m comment --comment "manually replaced rule 2" -j ACCEPT
+-A FORWARD -m comment --comment "rule 3" -j ACCEPT
+-A FORWARD -m comment --comment "appended rule 3" -j ACCEPT'
+
+diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
+
+# test rule replacement in dump files
+
+$XT_MULTI iptables-restore <<EOF
+*filter
+-A FORWARD -m comment --comment "rule 1" -j ACCEPT
+-A FORWARD -m comment --comment "rule to be replaced" -j ACCEPT
+-A FORWARD -m comment --comment "rule 3" -j ACCEPT
+COMMIT
+EOF
+
+$XT_MULTI iptables-restore --noflush <<EOF
+*filter
+-R FORWARD 2 -m comment --comment "replacement" -j ACCEPT
+-I FORWARD 2 -m comment --comment "insert referencing replaced rule" -j ACCEPT
+COMMIT
+EOF
+
+EXPECT='-A FORWARD -m comment --comment "rule 1" -j ACCEPT
+-A FORWARD -m comment --comment "insert referencing replaced rule" -j ACCEPT
+-A FORWARD -m comment --comment replacement -j ACCEPT
+-A FORWARD -m comment --comment "rule 3" -j ACCEPT'
+
+diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
diff --git a/iptables/tests/shell/testcases/iptables/0005-rule-replace_0 b/iptables/tests/shell/testcases/iptables/0005-rule-replace_0
new file mode 100755
index 0000000000000..5a3e922e50672
--- /dev/null
+++ b/iptables/tests/shell/testcases/iptables/0005-rule-replace_0
@@ -0,0 +1,38 @@ 
+#!/bin/bash
+
+# test rule replacement
+
+set -e
+
+# show rules, drop uninteresting policy settings
+ipt_show() {
+	$XT_MULTI iptables -S | grep -v '^-P'
+}
+
+$XT_MULTI iptables -A FORWARD -m comment --comment "rule 1" -j ACCEPT
+$XT_MULTI iptables -A FORWARD -m comment --comment "rule 2" -j ACCEPT
+$XT_MULTI iptables -A FORWARD -m comment --comment "rule 3" -j ACCEPT
+
+$XT_MULTI iptables -R FORWARD 2 -m comment --comment "replaced 2" -j ACCEPT
+
+EXPECT='-A FORWARD -m comment --comment "rule 1" -j ACCEPT
+-A FORWARD -m comment --comment "replaced 2" -j ACCEPT
+-A FORWARD -m comment --comment "rule 3" -j ACCEPT'
+
+diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
+
+$XT_MULTI iptables -R FORWARD 1 -m comment --comment "replaced 1" -j ACCEPT
+
+EXPECT='-A FORWARD -m comment --comment "replaced 1" -j ACCEPT
+-A FORWARD -m comment --comment "replaced 2" -j ACCEPT
+-A FORWARD -m comment --comment "rule 3" -j ACCEPT'
+
+diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
+
+$XT_MULTI iptables -R FORWARD 3 -m comment --comment "replaced 3" -j ACCEPT
+
+EXPECT='-A FORWARD -m comment --comment "replaced 1" -j ACCEPT
+-A FORWARD -m comment --comment "replaced 2" -j ACCEPT
+-A FORWARD -m comment --comment "replaced 3" -j ACCEPT'
+
+diff -u -Z <(echo -e "$EXPECT") <(ipt_show)