[iptables PATCH 1/4] *tables-restore: Enforce correct counters syntax if present

[iptables PATCH 1/4] *tables-restore: Enforce correct counters syntax if present

[Phil Sutter]

If '--counters' option was not given, restore parsers would ignore
anything following the policy word. Make them more strict, rejecting
anything in that spot which does not look like counter values even if
not restoring counters.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/iptables-restore.c                   | 20 +++++++++----------
 .../ipt-restore/0008-restore-counters_0       |  7 +++++++
 iptables/xtables-restore.c                    | 18 ++++++++---------
 3 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index 6f7ddf93b01bb..f11b2dc2fd316 100644
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -283,23 +283,21 @@ ip46tables_restore_main(const struct iptables_restore_cb *cb,
 					      xt_params->program_name, line);
 
 			if (strcmp(policy, "-") != 0) {
+				char *ctrs = strtok(NULL, " \t\n");
 				struct xt_counters count = {};
 
-				if (counters) {
-					char *ctrs;
-					ctrs = strtok(NULL, " \t\n");
-
-					if (!ctrs || !parse_counters(ctrs, &count))
-						xtables_error(PARAMETER_PROBLEM,
-							      "invalid policy counters for chain '%s'",
-							      chain);
-				}
+				if ((!ctrs && counters) ||
+				    (ctrs && !parse_counters(ctrs, &count)))
+					xtables_error(PARAMETER_PROBLEM,
+						      "invalid policy counters for chain '%s'",
+						      chain);
 
 				DEBUGP("Setting policy of chain %s to %s\n",
 					chain, policy);
 
-				if (!cb->ops->set_policy(chain, policy, &count,
-						     handle))
+				if (!cb->ops->set_policy(chain, policy,
+							 counters ? &count : NULL,
+							 handle))
 					xtables_error(OTHER_PROBLEM,
 						      "Can't set policy `%s' on `%s' line %u: %s",
 						      policy, chain, line,
diff --git a/iptables/tests/shell/testcases/ipt-restore/0008-restore-counters_0 b/iptables/tests/shell/testcases/ipt-restore/0008-restore-counters_0
index 5ac70682b76bf..854768c96e0da 100755
--- a/iptables/tests/shell/testcases/ipt-restore/0008-restore-counters_0
+++ b/iptables/tests/shell/testcases/ipt-restore/0008-restore-counters_0
@@ -20,3 +20,10 @@ EXPECT=":foo - [0:0]
 
 $XT_MULTI iptables-restore --counters <<< "$DUMP"
 diff -u -Z <(echo -e "$EXPECT") <($XT_MULTI iptables-save --counters | grep foo)
+
+# if present, counters must be in proper format
+! $XT_MULTI iptables-restore <<EOF
+*filter
+:FORWARD ACCEPT bar
+COMMIT
+EOF
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index abe56374289f4..23cd349819f4f 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -166,19 +166,17 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 				      xt_params->program_name, line);
 
 		if (nft_chain_builtin_find(state->curtable, chain)) {
-			if (counters) {
-				char *ctrs;
-				ctrs = strtok(NULL, " \t\n");
+			char *ctrs = strtok(NULL, " \t\n");
 
-				if (!ctrs || !parse_counters(ctrs, &count))
-					xtables_error(PARAMETER_PROBLEM,
-						      "invalid policy counters for chain '%s'",
-						      chain);
-
-			}
+			if ((!ctrs && counters) ||
+			    (ctrs && !parse_counters(ctrs, &count)))
+				xtables_error(PARAMETER_PROBLEM,
+					      "invalid policy counters for chain '%s'",
+					      chain);
 			if (cb->chain_set &&
 			    cb->chain_set(h, state->curtable->name,
-					  chain, policy, &count) < 0) {
+					  chain, policy,
+					  counters ? &count : NULL) < 0) {
 				xtables_error(OTHER_PROBLEM,
 					      "Can't set policy `%s' on `%s' line %u: %s",
 					      policy, chain, line,
-- 
2.40.0

[iptables PATCH 2/4] *tables: Reject invalid chain names when renaming

[Phil Sutter]

While given chain name was sanity checked with --new-chain command,
--rename-chain command allowed to choose an invalid name. Keep things
consistent by adding the missing check.

Fixes: e6869a8f59d77 ("reorganized tree after kernel merge")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xshared.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/iptables/xshared.c b/iptables/xshared.c
index 28c65faed7b25..5f75a0a57a023 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -1511,6 +1511,7 @@ void do_parse(int argc, char *argv[],
 					   "-%c requires old-chain-name and "
 					   "new-chain-name",
 					    cmd2char(CMD_RENAME_CHAIN));
+			assert_valid_chain_name(p->newname);
 			break;
 
 		case 'P':
-- 
2.40.0

[iptables PATCH 3/4] ebtables: Improve invalid chain name detection

[Phil Sutter]

Fix several issues:

- Most importantly, --new-chain command accepted any name. Introduce
  ebt_assert_valid_chain_name() for use with both --new-chain and
  --rename-chain.
- Restrict maximum name length to what legacy ebtables allows - this is
  a bit more than iptables-nft, subject to be unified.
- Like iptables, legacy ebtables rejects names prefixed by '-' or '!'.
- Use xs_has_arg() for consistency, keep the check for extra args for
  now.

Fixes: da871de2a6efb ("nft: bootstrap ebtables-compat")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xtables-eb.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index bf35f52b7585d..08eec79d80400 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -42,6 +42,10 @@
 #include "nft.h"
 #include "nft-bridge.h"
 
+/* from linux/netfilter_bridge/ebtables.h */
+#define EBT_TABLE_MAXNAMELEN 32
+#define EBT_CHAIN_MAXNAMELEN EBT_TABLE_MAXNAMELEN
+
 /*
  * From include/ebtables_u.h
  */
@@ -74,6 +78,26 @@ static int ebt_check_inverse2(const char option[], int argc, char **argv)
 	return ebt_invert;
 }
 
+/* XXX: merge with assert_valid_chain_name()? */
+static void ebt_assert_valid_chain_name(const char *chainname)
+{
+	if (strlen(chainname) >= EBT_CHAIN_MAXNAMELEN)
+		xtables_error(PARAMETER_PROBLEM,
+			      "Chain name length can't exceed %d",
+			      EBT_CHAIN_MAXNAMELEN - 1);
+
+	if (*chainname == '-' || *chainname == '!')
+		xtables_error(PARAMETER_PROBLEM, "No chain name specified");
+
+	if (xtables_find_target(chainname, XTF_TRY_LOAD))
+		xtables_error(PARAMETER_PROBLEM,
+			      "Target with name %s exists", chainname);
+
+	if (strchr(chainname, ' ') != NULL)
+		xtables_error(PARAMETER_PROBLEM,
+			      "Use of ' ' not allowed in chain names");
+}
+
 /*
  * Glue code to use libxtables
  */
@@ -751,6 +775,7 @@ int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table,
 			flags |= OPT_COMMAND;
 
 			if (c == 'N') {
+				ebt_assert_valid_chain_name(chain);
 				ret = nft_cmd_chain_user_add(h, chain, *table);
 				break;
 			} else if (c == 'X') {
@@ -764,14 +789,12 @@ int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table,
 			}
 
 			if (c == 'E') {
-				if (optind >= argc)
+				if (!xs_has_arg(argc, argv))
 					xtables_error(PARAMETER_PROBLEM, "No new chain name specified");
 				else if (optind < argc - 1)
 					xtables_error(PARAMETER_PROBLEM, "No extra options allowed with -E");
-				else if (strlen(argv[optind]) >= NFT_CHAIN_MAXNAMELEN)
-					xtables_error(PARAMETER_PROBLEM, "Chain name length can't exceed %d"" characters", NFT_CHAIN_MAXNAMELEN - 1);
-				else if (strchr(argv[optind], ' ') != NULL)
-					xtables_error(PARAMETER_PROBLEM, "Use of ' ' not allowed in chain names");
+
+				ebt_assert_valid_chain_name(argv[optind]);
 
 				errno = 0;
 				ret = nft_cmd_chain_user_rename(h, chain, *table,
-- 
2.40.0

[iptables PATCH 4/4] tests: shell: Fix and extend chain rename test

[Phil Sutter]

The old version exited unintentionally before testing ip6tables. Replace
it by a more complete variant testing for all tools, creating and
renaming of,chains with various illegal names instead of just renaming
to a clashing name.

Fixes: ed9cfe1b48526 ("tests: add initial save/restore test cases")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 .../tests/shell/testcases/chain/0003rename_0  | 40 +++++++++++++++++++
 .../tests/shell/testcases/chain/0003rename_1  | 12 ------
 2 files changed, 40 insertions(+), 12 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/chain/0003rename_0
 delete mode 100755 iptables/tests/shell/testcases/chain/0003rename_1

diff --git a/iptables/tests/shell/testcases/chain/0003rename_0 b/iptables/tests/shell/testcases/chain/0003rename_0
new file mode 100755
index 0000000000000..4cb2745bc2523
--- /dev/null
+++ b/iptables/tests/shell/testcases/chain/0003rename_0
@@ -0,0 +1,40 @@
+#!/bin/bash -x
+
+die() {
+	echo "E: $@"
+	exit 1
+}
+
+cmds="iptables ip6tables"
+[[ $XT_MULTI == *xtables-nft-multi ]] && cmds+=" arptables ebtables"
+
+declare -A invnames
+invnames["existing"]="c2"
+invnames["spaced"]="foo bar"
+invnames["dashed"]="-foo"
+invnames["negated"]="!foo"
+# XXX: ebtables-nft accepts 255 chars
+#invnames["overlong"]="thisisquitealongnameforachain"
+invnames["standard target"]="ACCEPT"
+invnames["extension target"]="DNAT"
+
+for cmd in $cmds; do
+	$XT_MULTI $cmd -N c1 || die "$cmd: can't add chain c1"
+	$XT_MULTI $cmd -N c2 || die "$cmd: can't add chain c2"
+	for key in "${!invnames[@]}"; do
+		val="${invnames[$key]}"
+		if [[ $key == "extension target" ]]; then
+			if [[ $cmd == "arptables" ]]; then
+				val="mangle"
+			elif [[ $cmd == "ebtables" ]]; then
+				val="dnat"
+			fi
+		fi
+		$XT_MULTI $cmd -N "$val" && \
+			die "$cmd: added chain with $key name"
+		$XT_MULTI $cmd -E c1 "$val" && \
+			die "$cmd: renamed to $key name"
+	done
+done
+
+exit 0
diff --git a/iptables/tests/shell/testcases/chain/0003rename_1 b/iptables/tests/shell/testcases/chain/0003rename_1
deleted file mode 100755
index 975c8e196b9f5..0000000000000
--- a/iptables/tests/shell/testcases/chain/0003rename_1
+++ /dev/null
@@ -1,12 +0,0 @@
-#!/bin/bash
-
-$XT_MULTI iptables -N c1 || exit 0
-$XT_MULTI iptables -N c2 || exit 0
-$XT_MULTI iptables -E c1 c2 || exit 1
-
-$XT_MULTI ip6tables -N c1 || exit 0
-$XT_MULTI ip6tables -N c2 || exit 0
-$XT_MULTI ip6tables -E c1 c2 || exit 1
-
-echo "E: Renamed with existing chain" >&2
-exit 0
-- 
2.40.0

Re: [iptables PATCH 1/4] *tables-restore: Enforce correct counters syntax if present

[Phil Sutter]

On Fri, Jul 28, 2023 at 02:31:44PM +0200, Phil Sutter wrote:
> If '--counters' option was not given, restore parsers would ignore
> anything following the policy word. Make them more strict, rejecting
> anything in that spot which does not look like counter values even if
> not restoring counters.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Series applied.