| Message ID | 1495039661-9178-1-git-send-email-ojford@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Florian Westphal |
| Headers | show |
Oliver Ford <ojford@gmail.com> wrote: > Fixes the crash reported in Bugzilla #1131 where a malformed > parameter that specifies the table option can create an invalid > pointer. > > Improves the tables option check to find a beginning '-' followed by > a 't' anywhere in the parameter. > diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c > index 876fe06..70dc7eb 100644 > --- a/iptables/iptables-restore.c > +++ b/iptables/iptables-restore.c > @@ -162,8 +162,7 @@ static void add_param_to_argv(char *parsestart) > param_buffer[param_len] = '\0'; > > /* check if table name specified */ > - if (!strncmp(param_buffer, "-t", 2) > - || !strncmp(param_buffer, "--table", 8)) { > + if (param_buffer[0] == '-' && strchr(param_buffer, 't')) { > xtables_error(PARAMETER_PROBLEM, > "The -t option (seen in line %u) cannot be " > "used in iptables-restore.\n", line); strchr() can only called if param_buffer[1] != '-', else this breaks something like: # Generated by iptables-save v1.6.0 on Thu May 18 00:00:38 2017 *filter :INPUT ACCEPT [0:0] :FORWARD ACCEPT [0:0] :OUTPUT ACCEPT [0:0] --delete foo COMMIT (use '-n' option to iptables). -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 17, 2017 at 11:19 PM, Florian Westphal <fw@strlen.de> wrote: > Oliver Ford <ojford@gmail.com> wrote: >> Fixes the crash reported in Bugzilla #1131 where a malformed >> parameter that specifies the table option can create an invalid >> pointer. >> >> Improves the tables option check to find a beginning '-' followed by >> a 't' anywhere in the parameter. > >> diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c >> index 876fe06..70dc7eb 100644 >> --- a/iptables/iptables-restore.c >> +++ b/iptables/iptables-restore.c >> @@ -162,8 +162,7 @@ static void add_param_to_argv(char *parsestart) >> param_buffer[param_len] = '\0'; >> >> /* check if table name specified */ >> - if (!strncmp(param_buffer, "-t", 2) >> - || !strncmp(param_buffer, "--table", 8)) { >> + if (param_buffer[0] == '-' && strchr(param_buffer, 't')) { >> xtables_error(PARAMETER_PROBLEM, >> "The -t option (seen in line %u) cannot be " >> "used in iptables-restore.\n", line); > > strchr() can only called if param_buffer[1] != '-', else this breaks > something like: > > # Generated by iptables-save v1.6.0 on Thu May 18 00:00:38 2017 > *filter > :INPUT ACCEPT [0:0] > :FORWARD ACCEPT [0:0] > :OUTPUT ACCEPT [0:0] > --delete foo > COMMIT > > (use '-n' option to iptables). Good point I'll check for that and send an updated patch. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Oliver Ford <ojford@gmail.com> wrote: > On Wed, May 17, 2017 at 11:19 PM, Florian Westphal <fw@strlen.de> wrote: > > Oliver Ford <ojford@gmail.com> wrote: > >> Fixes the crash reported in Bugzilla #1131 where a malformed > >> parameter that specifies the table option can create an invalid > >> pointer. > >> > >> Improves the tables option check to find a beginning '-' followed by > >> a 't' anywhere in the parameter. > > > >> diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c > >> index 876fe06..70dc7eb 100644 > >> --- a/iptables/iptables-restore.c > >> +++ b/iptables/iptables-restore.c > >> @@ -162,8 +162,7 @@ static void add_param_to_argv(char *parsestart) > >> param_buffer[param_len] = '\0'; > >> > >> /* check if table name specified */ > >> - if (!strncmp(param_buffer, "-t", 2) > >> - || !strncmp(param_buffer, "--table", 8)) { > >> + if (param_buffer[0] == '-' && strchr(param_buffer, 't')) { > >> xtables_error(PARAMETER_PROBLEM, > >> "The -t option (seen in line %u) cannot be " > >> "used in iptables-restore.\n", line); > > > > strchr() can only called if param_buffer[1] != '-', else this breaks > > something like: > > > > # Generated by iptables-save v1.6.0 on Thu May 18 00:00:38 2017 > > *filter > > :INPUT ACCEPT [0:0] > > :FORWARD ACCEPT [0:0] > > :OUTPUT ACCEPT [0:0] > > --delete foo > > COMMIT > > > > (use '-n' option to iptables). > > Good point I'll check for that and send an updated patch. Forgot to mention: Would be nice to explain in the commit message why this is needed, e.g. -nt, --tab, etc. are not caught by current test. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/iptables/ip6tables-restore.c b/iptables/ip6tables-restore.c index 39a881d..1a8cc5e 100644 --- a/iptables/ip6tables-restore.c +++ b/iptables/ip6tables-restore.c @@ -165,8 +165,7 @@ static void add_param_to_argv(char *parsestart) param_buffer[param_len] = '\0'; /* check if table name specified */ - if (!strncmp(param_buffer, "-t", 2) - || !strncmp(param_buffer, "--table", 8)) { + if (param_buffer[0] == '-' && strchr(param_buffer, 't')) { xtables_error(PARAMETER_PROBLEM, "The -t option (seen in line %u) cannot be " "used in ip6tables-restore.\n", line); diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c index 876fe06..70dc7eb 100644 --- a/iptables/iptables-restore.c +++ b/iptables/iptables-restore.c @@ -162,8 +162,7 @@ static void add_param_to_argv(char *parsestart) param_buffer[param_len] = '\0'; /* check if table name specified */ - if (!strncmp(param_buffer, "-t", 2) - || !strncmp(param_buffer, "--table", 8)) { + if (param_buffer[0] == '-' && strchr(param_buffer, 't')) { xtables_error(PARAMETER_PROBLEM, "The -t option (seen in line %u) cannot be " "used in iptables-restore.\n", line); diff --git a/iptables/iptables-xml.c b/iptables/iptables-xml.c index 2e093b5..fe219ae 100644 --- a/iptables/iptables-xml.c +++ b/iptables/iptables-xml.c @@ -819,13 +819,11 @@ iptables_xml_main(int argc, char *argv[]) *(param_buffer + param_len) = '\0'; /* check if table name specified */ - if (!strncmp(param_buffer, "-t", 3) - || !strncmp(param_buffer, - "--table", 8)) { + if (param_buffer[0] == '-' && strchr(param_buffer, 't')) { xtables_error(PARAMETER_PROBLEM, - "Line %u seems to have a " - "-t table option.\n", - line); + "Line %u seems to have a " + "-t table option.\n", + line); exit(1); } diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c index 15824f0..75a64a7 100644 --- a/iptables/xtables-restore.c +++ b/iptables/xtables-restore.c @@ -136,8 +136,7 @@ static void add_param_to_argv(char *parsestart) param_buffer[param_len] = '\0'; /* check if table name specified */ - if (!strncmp(param_buffer, "-t", 2) - || !strncmp(param_buffer, "--table", 8)) { + if (param_buffer[0] == '-' && strchr(param_buffer, 't')) { xtables_error(PARAMETER_PROBLEM, "The -t option (seen in line %u) cannot be " "used in xtables-restore.\n", line);
Fixes the crash reported in Bugzilla #1131 where a malformed parameter that specifies the table option can create an invalid pointer. Improves the tables option check to find a beginning '-' followed by a 't' anywhere in the parameter. Signed-off-by: Oliver Ford <ojford@gmail.com> --- iptables/ip6tables-restore.c | 3 +-- iptables/iptables-restore.c | 3 +-- iptables/iptables-xml.c | 10 ++++------ iptables/xtables-restore.c | 3 +-- 4 files changed, 7 insertions(+), 12 deletions(-)