[PATCH 1/1] iptables: Fix crash on malformed iptables-restore

[PATCH 1/1] iptables: Fix crash on malformed iptables-restore

[Oliver Ford]

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(-)

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);
-- 
2.11.0

Re: [1/1] iptables: Fix crash on malformed iptables-restore

[Florian Westphal]

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).

Re: [1/1] iptables: Fix crash on malformed iptables-restore

[Oliver Ford]

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.

Re: [1/1] iptables: Fix crash on malformed iptables-restore

[Florian Westphal]

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.