| 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 | expand |
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
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.
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
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.
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
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 */
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;
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