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

[PATCH v3 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 during a restore can create an invalid pointer.
It was discovered during fuzz testing that options like '-ftf'
can cause a segfault. A parameter that includes a 't' is not currently
filtered correctly.

Improves the filtering to:
Filter a beginning '-' followed by a character other than '-' and then a 't'
anywhere in the parameter. This filters parameters like '-ftf'.
Filter a beginning '--t'. Because the getopt_long function allows abbreviations,
any parameter beginning with '--t' will be treated as '--table'. As no other
iptables option begins with a 't' this won't filter out other options.

Signed-off-by: Oliver Ford <ojford@gmail.com>
---
 iptables/ip6tables-restore.c |  9 +++++----
 iptables/iptables-restore.c  |  9 +++++----
 iptables/iptables-xml.c      | 11 +++++------
 iptables/xtables-restore.c   |  9 +++++----
 4 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/iptables/ip6tables-restore.c b/iptables/ip6tables-restore.c
index 39a881d..46e9fae 100644
--- a/iptables/ip6tables-restore.c
+++ b/iptables/ip6tables-restore.c
@@ -165,11 +165,12 @@ 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] == '-' && param_buffer[1] != '-'
+				&& strchr(param_buffer, 't'))
+				|| !strncmp(param_buffer, "--t", 3)) {
 				xtables_error(PARAMETER_PROBLEM,
-				"The -t option (seen in line %u) cannot be "
-				"used in ip6tables-restore.\n", line);
+					"The -t option (seen in line %u) cannot be "
+					"used in ip6tables-restore.\n", line);
 				exit(1);
 			}
 
diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index 876fe06..9b06395 100644
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -162,11 +162,12 @@ 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] == '-' && param_buffer[1] != '-'
+				&& strchr(param_buffer, 't'))
+				|| !strncmp(param_buffer, "--t", 3)) {
 				xtables_error(PARAMETER_PROBLEM,
-				"The -t option (seen in line %u) cannot be "
-				"used in iptables-restore.\n", line);
+					"The -t option (seen in line %u) cannot be "
+					"used in iptables-restore.\n", line);
 				exit(1);
 			}
 
diff --git a/iptables/iptables-xml.c b/iptables/iptables-xml.c
index 2e093b5..d836ff4 100644
--- a/iptables/iptables-xml.c
+++ b/iptables/iptables-xml.c
@@ -819,13 +819,12 @@ 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] == '-' && param_buffer[1] != '-'
+						&& strchr(param_buffer, 't'))
+						|| !strncmp(param_buffer, "--t", 3)) {
 						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..994e04e 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -136,11 +136,12 @@ 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] == '-' && param_buffer[1] != '-'
+				&& strchr(param_buffer, 't'))
+				|| !strncmp(param_buffer, "--t", 3)) {
 				xtables_error(PARAMETER_PROBLEM,
-				"The -t option (seen in line %u) cannot be "
-				"used in xtables-restore.\n", line);
+					"The -t option (seen in line %u) cannot be "
+					"used in xtables-restore.\n", line);
 				exit(1);
 			}
 
-- 
2.11.0

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

[Florian Westphal]

Oliver Ford <ojford@gmail.com> wrote:
> Filter a beginning '--t'. Because the getopt_long function allows abbreviations,
> any parameter beginning with '--t' will be treated as '--table'.

No, thats not correct:
--t is treated as --table.
--tfoo is an invalid option.
--ttl is ttl.

So this:

> +				|| !strncmp(param_buffer, "--t", 3)) {
>  				xtables_error(PARAMETER_PROBLEM,
> +					"The -t option (seen in line %u) cannot be "
> +					"used in ip6tables-restore.\n", line);

.. rejects rules like

-A INPUT -m ttl --ttl 32

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

[Oliver Ford]

On Fri, May 19, 2017 at 11:04 AM, Florian Westphal <fw@strlen.de> wrote:
> Oliver Ford <ojford@gmail.com> wrote:
>> Filter a beginning '--t'. Because the getopt_long function allows abbreviations,
>> any parameter beginning with '--t' will be treated as '--table'.
>
> No, thats not correct:
> --t is treated as --table.
> --tfoo is an invalid option.
> --ttl is ttl.
>
> So this:
>
>> +                             || !strncmp(param_buffer, "--t", 3)) {
>>                               xtables_error(PARAMETER_PROBLEM,
>> +                                     "The -t option (seen in line %u) cannot be "
>> +                                     "used in ip6tables-restore.\n", line);
>
> .. rejects rules like
>
> -A INPUT -m ttl --ttl 32

Would strncmp(param_buffer, "--ta", 4) work? I don't think there are
any options that begin with --ta other than --table.

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

[Florian Westphal]

Oliver Ford <ojford@gmail.com> wrote:
> On Fri, May 19, 2017 at 11:04 AM, Florian Westphal <fw@strlen.de> wrote:
> > Oliver Ford <ojford@gmail.com> wrote:
> >> Filter a beginning '--t'. Because the getopt_long function allows abbreviations,
> >> any parameter beginning with '--t' will be treated as '--table'.
> >
> > No, thats not correct:
> > --t is treated as --table.
> > --tfoo is an invalid option.
> > --ttl is ttl.
> >
> > So this:
> >
> >> +                             || !strncmp(param_buffer, "--t", 3)) {
> >>                               xtables_error(PARAMETER_PROBLEM,
> >> +                                     "The -t option (seen in line %u) cannot be "
> >> +                                     "used in ip6tables-restore.\n", line);
> >
> > .. rejects rules like
> >
> > -A INPUT -m ttl --ttl 32
> 
> Would strncmp(param_buffer, "--ta", 4) work? I don't think there are
> any options that begin with --ta other than --table.

That won't catch '--t'.

It will also add trouble later if any module adds an option like --tap,
--tail, --target, etc.

Whats wrong with:

if ((param_buffer[0] == '-' && param_buffer[1] != '-' &&
     strchr(param_buffer, 't') ||
     (!strncmp(param_buffer, "--t", 3) &&
      !strncmp(param_buffer, "--table", strlen(param_buffer)))) {

?

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

[Oliver Ford]

On Fri, May 19, 2017 at 11:38 AM, Florian Westphal <fw@strlen.de> wrote:
> Oliver Ford <ojford@gmail.com> wrote:
>> On Fri, May 19, 2017 at 11:04 AM, Florian Westphal <fw@strlen.de> wrote:
>> > Oliver Ford <ojford@gmail.com> wrote:
>> >> Filter a beginning '--t'. Because the getopt_long function allows abbreviations,
>> >> any parameter beginning with '--t' will be treated as '--table'.
>> >
>> > No, thats not correct:
>> > --t is treated as --table.
>> > --tfoo is an invalid option.
>> > --ttl is ttl.
>> >
>> > So this:
>> >
>> >> +                             || !strncmp(param_buffer, "--t", 3)) {
>> >>                               xtables_error(PARAMETER_PROBLEM,
>> >> +                                     "The -t option (seen in line %u) cannot be "
>> >> +                                     "used in ip6tables-restore.\n", line);
>> >
>> > .. rejects rules like
>> >
>> > -A INPUT -m ttl --ttl 32
>>
>> Would strncmp(param_buffer, "--ta", 4) work? I don't think there are
>> any options that begin with --ta other than --table.
>
> That won't catch '--t'.
>
> It will also add trouble later if any module adds an option like --tap,
> --tail, --target, etc.
>
> Whats wrong with:
>
> if ((param_buffer[0] == '-' && param_buffer[1] != '-' &&
>      strchr(param_buffer, 't') ||
>      (!strncmp(param_buffer, "--t", 3) &&
>       !strncmp(param_buffer, "--table", strlen(param_buffer)))) {
>
> ?

Yes I think that will work. Will test and resend.

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

[Oliver Ford]

On Fri, May 19, 2017 at 11:38 AM, Florian Westphal <fw@strlen.de> wrote:
> Oliver Ford <ojford@gmail.com> wrote:
>> On Fri, May 19, 2017 at 11:04 AM, Florian Westphal <fw@strlen.de> wrote:
>> > Oliver Ford <ojford@gmail.com> wrote:
>> >> Filter a beginning '--t'. Because the getopt_long function allows abbreviations,
>> >> any parameter beginning with '--t' will be treated as '--table'.
>> >
>> > No, thats not correct:
>> > --t is treated as --table.
>> > --tfoo is an invalid option.
>> > --ttl is ttl.
>> >
>> > So this:
>> >
>> >> +                             || !strncmp(param_buffer, "--t", 3)) {
>> >>                               xtables_error(PARAMETER_PROBLEM,
>> >> +                                     "The -t option (seen in line %u) cannot be "
>> >> +                                     "used in ip6tables-restore.\n", line);
>> >
>> > .. rejects rules like
>> >
>> > -A INPUT -m ttl --ttl 32
>>
>> Would strncmp(param_buffer, "--ta", 4) work? I don't think there are
>> any options that begin with --ta other than --table.
>
> That won't catch '--t'.
>
> It will also add trouble later if any module adds an option like --tap,
> --tail, --target, etc.
>
> Whats wrong with:
>
> if ((param_buffer[0] == '-' && param_buffer[1] != '-' &&
>      strchr(param_buffer, 't') ||
>      (!strncmp(param_buffer, "--t", 3) &&
>       !strncmp(param_buffer, "--table", strlen(param_buffer)))) {
>
> ?

I've just sent v4 that definitely works now. If you've got
"!strncmp(param_buffer, "--table", strlen(param_buffer))" you don't
also need "!strncmp(param_buffer, "--t", 3)" as --t will get filtered.

I've tested --t --ta --tab --tabl --table gets filtered but not --ttl,
--target, --type.

Also -ftf still gets filtered. Thanks for bearing with this.

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

[Florian Westphal]

Oliver Ford <ojford@gmail.com> wrote:
> On Fri, May 19, 2017 at 11:38 AM, Florian Westphal <fw@strlen.de> wrote:
> > Oliver Ford <ojford@gmail.com> wrote:
> >> On Fri, May 19, 2017 at 11:04 AM, Florian Westphal <fw@strlen.de> wrote:
> >> > Oliver Ford <ojford@gmail.com> wrote:
> >> >> Filter a beginning '--t'. Because the getopt_long function allows abbreviations,
> >> >> any parameter beginning with '--t' will be treated as '--table'.
> >> >
> >> > No, thats not correct:
> >> > --t is treated as --table.
> >> > --tfoo is an invalid option.
> >> > --ttl is ttl.
> >> >
> >> > So this:
> >> >
> >> >> +                             || !strncmp(param_buffer, "--t", 3)) {
> >> >>                               xtables_error(PARAMETER_PROBLEM,
> >> >> +                                     "The -t option (seen in line %u) cannot be "
> >> >> +                                     "used in ip6tables-restore.\n", line);
> >> >
> >> > .. rejects rules like
> >> >
> >> > -A INPUT -m ttl --ttl 32
> >>
> >> Would strncmp(param_buffer, "--ta", 4) work? I don't think there are
> >> any options that begin with --ta other than --table.
> >
> > That won't catch '--t'.
> >
> > It will also add trouble later if any module adds an option like --tap,
> > --tail, --target, etc.
> >
> > Whats wrong with:
> >
> > if ((param_buffer[0] == '-' && param_buffer[1] != '-' &&
> >      strchr(param_buffer, 't') ||
> >      (!strncmp(param_buffer, "--t", 3) &&
> >       !strncmp(param_buffer, "--table", strlen(param_buffer)))) {
> >
> > ?
> 
> I've just sent v4 that definitely works now. If you've got
> "!strncmp(param_buffer, "--table", strlen(param_buffer))" you don't
> also need "!strncmp(param_buffer, "--t", 3)" as --t will get filtered.

Its needed, else '--' marker gets detected as 'table', e.g.:
-A INPUT -m ttl --ttl 32 -j ACCEPT --

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

[Oliver Ford]

On Fri, May 19, 2017 at 12:36 PM, Florian Westphal <fw@strlen.de> wrote:
> Oliver Ford <ojford@gmail.com> wrote:
>> On Fri, May 19, 2017 at 11:38 AM, Florian Westphal <fw@strlen.de> wrote:
>> > Oliver Ford <ojford@gmail.com> wrote:
>> >> On Fri, May 19, 2017 at 11:04 AM, Florian Westphal <fw@strlen.de> wrote:
>> >> > Oliver Ford <ojford@gmail.com> wrote:
>> >> >> Filter a beginning '--t'. Because the getopt_long function allows abbreviations,
>> >> >> any parameter beginning with '--t' will be treated as '--table'.
>> >> >
>> >> > No, thats not correct:
>> >> > --t is treated as --table.
>> >> > --tfoo is an invalid option.
>> >> > --ttl is ttl.
>> >> >
>> >> > So this:
>> >> >
>> >> >> +                             || !strncmp(param_buffer, "--t", 3)) {
>> >> >>                               xtables_error(PARAMETER_PROBLEM,
>> >> >> +                                     "The -t option (seen in line %u) cannot be "
>> >> >> +                                     "used in ip6tables-restore.\n", line);
>> >> >
>> >> > .. rejects rules like
>> >> >
>> >> > -A INPUT -m ttl --ttl 32
>> >>
>> >> Would strncmp(param_buffer, "--ta", 4) work? I don't think there are
>> >> any options that begin with --ta other than --table.
>> >
>> > That won't catch '--t'.
>> >
>> > It will also add trouble later if any module adds an option like --tap,
>> > --tail, --target, etc.
>> >
>> > Whats wrong with:
>> >
>> > if ((param_buffer[0] == '-' && param_buffer[1] != '-' &&
>> >      strchr(param_buffer, 't') ||
>> >      (!strncmp(param_buffer, "--t", 3) &&
>> >       !strncmp(param_buffer, "--table", strlen(param_buffer)))) {
>> >
>> > ?
>>
>> I've just sent v4 that definitely works now. If you've got
>> "!strncmp(param_buffer, "--table", strlen(param_buffer))" you don't
>> also need "!strncmp(param_buffer, "--t", 3)" as --t will get filtered.
>
> Its needed, else '--' marker gets detected as 'table', e.g.:
> -A INPUT -m ttl --ttl 32 -j ACCEPT --

Ok I've put that back in in v5 and just sent it.