[iptables] xtables-restore: Fix --table parameter check
diff mbox series

Message ID 20190920154920.7927-1-phil@nwl.cc
State Changes Requested
Delegated to: Pablo Neira
Headers show
Series
  • [iptables] xtables-restore: Fix --table parameter check
Related show

Commit Message

Phil Sutter Sept. 20, 2019, 3:49 p.m. UTC
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

Comments

Phil Sutter Oct. 18, 2019, 1:27 p.m. UTC | #1
Hi,

On Fri, Sep 20, 2019 at 05:49:20PM +0200, Phil Sutter wrote:
> 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>

Could anyone please review this one?

Thanks, Phil
Florian Westphal Oct. 18, 2019, 2:05 p.m. UTC | #2
Phil Sutter <phil@nwl.cc> wrote:
> 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.

Thanks for adding a test for this.
How did you generate it?  The added code is pure voodoo magic to me,
so I wonder if we can just remove the 'test for -t in iptables-restore
files' code.
Phil Sutter Oct. 18, 2019, 2:48 p.m. UTC | #3
Hi Florian,

On Fri, Oct 18, 2019 at 04:05:08PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > 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.
> 
> Thanks for adding a test for this.
> How did you generate it?  The added code is pure voodoo magic to me,
> so I wonder if we can just remove the 'test for -t in iptables-restore
> files' code.

Sorry, I didn't mean to create such unreadable code. I guess after
managing to wrap my head around to understand the old code, the new one
seemed much more clear to me. ;)

The problem with dropping that check is the potential mess we get when
users add '-t' parameter to rules in dumps. While *tables-restore adds
'-t' option for the current table itself, arg parsing in at least
do_commandeb and do_commandx accepts multiple '-t' options (so the last
one wins).

Assuming that this checking for '-t' presence is a mess and we should
get rid of it, I can imagine two alternatives:

1) Disallow multiple '-t' options. A nice and easy solution, but not
   backwards compatible.
2) Make *tables-restore add the '-t' option last. This is a bit of a
   hack and will cause unexpected behaviour for users trying to add '-t'
   option in dumps.

What do you think? Or should I respin after adding a bunch of comments
to is_table_param() to make it more clear?

Thanks, Phil
Florian Westphal Oct. 18, 2019, 8:58 p.m. UTC | #4
Phil Sutter <phil@nwl.cc> wrote:
> > How did you generate it?  The added code is pure voodoo magic to me,
> > so I wonder if we can just remove the 'test for -t in iptables-restore
> > files' code.
> 
> Sorry, I didn't mean to create such unreadable code. I guess after
> managing to wrap my head around to understand the old code, the new one
> seemed much more clear to me. ;)

Fair enough, my main point was where the test cases come from, i.e.
did you see such rule dumps in the wild, or did you create this manually
to catch all corner cases?

I see you have a test for things like "-?t", so I wondered where that
came from.

> What do you think? Or should I respin after adding a bunch of comments
> to is_table_param() to make it more clear?

I think thats the best option, I don't have any objections at the check
per se given older iptables does this too.
Phil Sutter Oct. 19, 2019, 10:15 a.m. UTC | #5
Hi,

On Fri, Oct 18, 2019 at 10:58:08PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > > How did you generate it?  The added code is pure voodoo magic to me,
> > > so I wonder if we can just remove the 'test for -t in iptables-restore
> > > files' code.
> > 
> > Sorry, I didn't mean to create such unreadable code. I guess after
> > managing to wrap my head around to understand the old code, the new one
> > seemed much more clear to me. ;)
> 
> Fair enough, my main point was where the test cases come from, i.e.
> did you see such rule dumps in the wild, or did you create this manually
> to catch all corner cases?
> 
> I see you have a test for things like "-?t", so I wondered where that
> came from.

Ah! Originally this comes from a Red Hat BZ, not sure what reporter
actually tested with but as long as the comment started with a dash and
contained a 't' somewhere it would trigger the bug.

I wrote the test case along with the new implementation, searching for
things that could be mismatched. The '-?t' for instance is to make sure
combined short-options are matched correctly: Since '?' is not a valid
short option (at least not in iptables), this must not match as '-t'
option.

> > What do you think? Or should I respin after adding a bunch of comments
> > to is_table_param() to make it more clear?
> 
> I think thats the best option, I don't have any objections at the check
> per se given older iptables does this too.

I don't quite like this check, hence I don't overly cling to it. As you
see, checking for presence of an option in getopt() format is not easy
and we do that for every option of every rule in a dump. Maybe we should
really just append the explicit table param and accept that user's table
option is not rejected but simply ignored.

Cheers, Phil
Florian Westphal Oct. 19, 2019, 1:34 p.m. UTC | #6
Phil Sutter <phil@nwl.cc> wrote:
> I don't quite like this check, hence I don't overly cling to it. As you
> see, checking for presence of an option in getopt() format is not easy
> and we do that for every option of every rule in a dump. Maybe we should
> really just append the explicit table param and accept that user's table
> option is not rejected but simply ignored.

I'd propose that you just push this patch out, with a few addiotnal
comments.

E.g. test script could have

# First a few inputs that should not be mistaken
# for a "-t" option:
OKLINES=
...
# Variants of -t, --table, etc. including
# multiple, concatenated short options.
NONOLINES...

For the actual code I will leave it up to you, perhaps
just include examples, e.g.

/* must catch inputs like --tab=mangle, too */
if (index(s, '=')), ..

...

As for the last part, maybe either convert it to
a loop instead of goto, or at least return right away
in match case, i.e.

switch (*s) {
case 't': return true;
case ' ': return false; /* end of options */
case '\0': return false; /* no 't' found */

Patch
diff mbox series

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;