netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [iptables PATCH] xtables-restore: Fix --table parameter check
@ 2019-09-20 15:49 Phil Sutter
  2019-10-18 13:27 ` Phil Sutter
  2019-10-18 14:05 ` Florian Westphal
  0 siblings, 2 replies; 7+ messages in thread
From: Phil Sutter @ 2019-09-20 15:49 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Xtables-restore tries to reject rule commands in input which contain a
--table parameter (since it is adding this itself based on the previous
table line). Sadly getopt_long's flexibility makes it hard to get this
check right: Since the last fix, comments starting with a dash and
containing a 't' character somewhere later were rejected. Simple
example:

| *filter
| -A FORWARD -m comment --comment "- allow this one" -j ACCEPT
| COMMIT

To hopefully sort this once and for all, introduce is_table_param()
which should cover all possible variants of legal and illegal
parameters. Also add a test to make sure it does what it is supposed to.

Fixes: f8e5ebc5986bf ("iptables: Fix crash on malformed iptables-restore")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 .../ipt-restore/0009-table-name-comment_0     | 48 +++++++++++++++++++
 iptables/xshared.c                            | 44 ++++++++++++++---
 2 files changed, 86 insertions(+), 6 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/ipt-restore/0009-table-name-comment_0

diff --git a/iptables/tests/shell/testcases/ipt-restore/0009-table-name-comment_0 b/iptables/tests/shell/testcases/ipt-restore/0009-table-name-comment_0
new file mode 100755
index 0000000000000..71c8feffd5adf
--- /dev/null
+++ b/iptables/tests/shell/testcases/ipt-restore/0009-table-name-comment_0
@@ -0,0 +1,48 @@
+#!/bin/bash
+
+OKLINES="- some comment
+--asdf
+-asdf t
+-?t"
+
+NONOLINES="-t foo
+-t
+--table
+--table foo
+--table=foo
+-asdft
+-tasdf
+--tab=foo
+-dfetbl"
+
+to_dump() { # (comment)
+	echo "*filter"
+	echo "-A FORWARD -m comment --comment \"$@\" -j ACCEPT"
+	echo "COMMIT"
+}
+
+ret=0
+
+while read okline; do
+	$XT_MULTI iptables -A FORWARD -m comment --comment "$okline" -j ACCEPT || {
+		echo "iptables failed for comment '$okline'"
+		ret=1
+	}
+	to_dump "$okline" | $XT_MULTI iptables-restore || {
+		echo "iptables-restore failed for comment '$okline'"
+		ret=1
+	}
+done <<< "$OKLINES"
+
+while read nonoline; do
+	$XT_MULTI iptables -A FORWARD -m comment --comment "$nonoline" -j ACCEPT >/dev/null 2>&1 || {
+		echo "iptables accepted comment '$nonoline'"
+		ret=1
+	}
+	to_dump "$nonoline" | $XT_MULTI iptables-restore >/dev/null 2>&1 && {
+		echo "iptables-restore accepted comment '$nonoline'"
+		ret=1
+	}
+done <<< "$NONOLINES"
+
+exit $ret
diff --git a/iptables/xshared.c b/iptables/xshared.c
index 36a2ec5f193d3..faa21d6cd69af 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -446,6 +446,43 @@ static void add_param(struct xt_param_buf *param, const char *curchar)
 			      "Parameter too long!");
 }
 
+static bool is_table_param(const char *s)
+{
+	if (s[0] != '-')
+		return false;
+
+	/* it is an option */
+	switch (s[1]) {
+	case 't':
+		/* -t found */
+		return true;
+	case '-':
+		/* it is a long option */
+		if (s[2] != 't')
+			return false;
+		if (index(s, '='))
+			return !strncmp(s, "--table", index(s, '=') - s);
+		return !strncmp(s, "--table", 7);
+	default:
+		break;
+	}
+	/* short options may be combined, check if 't' is among them */
+next:
+	s++;
+	switch (*s) {
+	case 't':
+	case ' ':
+	case '\0':
+		break;
+	case 'a' ... 's':
+	case 'u' ... 'z':
+	case 'A' ... 'Z':
+	case '0' ... '9':
+		goto next;
+	}
+	return *s == 't';
+}
+
 void add_param_to_argv(char *parsestart, int line)
 {
 	int quote_open = 0, escaped = 0;
@@ -499,15 +536,10 @@ void add_param_to_argv(char *parsestart, int line)
 		param.buffer[param.len] = '\0';
 
 		/* check if table name specified */
-		if ((param.buffer[0] == '-' &&
-		     param.buffer[1] != '-' &&
-		     strchr(param.buffer, 't')) ||
-		    (!strncmp(param.buffer, "--t", 3) &&
-		     !strncmp(param.buffer, "--table", strlen(param.buffer)))) {
+		if (is_table_param(param.buffer))
 			xtables_error(PARAMETER_PROBLEM,
 				      "The -t option (seen in line %u) cannot be used in %s.\n",
 				      line, xt_params->program_name);
-		}
 
 		add_argv(param.buffer, 0);
 		param.len = 0;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-10-19 13:34 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-09-20 15:49 [iptables PATCH] xtables-restore: Fix --table parameter check Phil Sutter
2019-10-18 13:27 ` Phil Sutter
2019-10-18 14:05 ` Florian Westphal
2019-10-18 14:48   ` Phil Sutter
2019-10-18 20:58     ` Florian Westphal
2019-10-19 10:15       ` Phil Sutter
2019-10-19 13:34         ` Florian Westphal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).