* [PATCH v3 1/1] iptables: Fix crash on malformed iptables-restore
@ 2017-05-19 9:54 Oliver Ford
2017-05-19 10:04 ` Florian Westphal
0 siblings, 1 reply; 8+ messages in thread
From: Oliver Ford @ 2017-05-19 9:54 UTC (permalink / raw)
To: netfilter-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v3 1/1] iptables: Fix crash on malformed iptables-restore
2017-05-19 9:54 [PATCH v3 1/1] iptables: Fix crash on malformed iptables-restore Oliver Ford
@ 2017-05-19 10:04 ` Florian Westphal
2017-05-19 10:12 ` Oliver Ford
0 siblings, 1 reply; 8+ messages in thread
From: Florian Westphal @ 2017-05-19 10:04 UTC (permalink / raw)
To: Oliver Ford; +Cc: netfilter-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3 1/1] iptables: Fix crash on malformed iptables-restore
2017-05-19 10:04 ` Florian Westphal
@ 2017-05-19 10:12 ` Oliver Ford
2017-05-19 10:38 ` Florian Westphal
0 siblings, 1 reply; 8+ messages in thread
From: Oliver Ford @ 2017-05-19 10:12 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3 1/1] iptables: Fix crash on malformed iptables-restore
2017-05-19 10:12 ` Oliver Ford
@ 2017-05-19 10:38 ` Florian Westphal
2017-05-19 10:41 ` Oliver Ford
2017-05-19 11:24 ` Oliver Ford
0 siblings, 2 replies; 8+ messages in thread
From: Florian Westphal @ 2017-05-19 10:38 UTC (permalink / raw)
To: Oliver Ford; +Cc: Florian Westphal, netfilter-devel
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)))) {
?
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3 1/1] iptables: Fix crash on malformed iptables-restore
2017-05-19 10:38 ` Florian Westphal
@ 2017-05-19 10:41 ` Oliver Ford
2017-05-19 11:24 ` Oliver Ford
1 sibling, 0 replies; 8+ messages in thread
From: Oliver Ford @ 2017-05-19 10:41 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3 1/1] iptables: Fix crash on malformed iptables-restore
2017-05-19 10:38 ` Florian Westphal
2017-05-19 10:41 ` Oliver Ford
@ 2017-05-19 11:24 ` Oliver Ford
2017-05-19 11:36 ` Florian Westphal
1 sibling, 1 reply; 8+ messages in thread
From: Oliver Ford @ 2017-05-19 11:24 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3 1/1] iptables: Fix crash on malformed iptables-restore
2017-05-19 11:24 ` Oliver Ford
@ 2017-05-19 11:36 ` Florian Westphal
2017-05-19 12:03 ` Oliver Ford
0 siblings, 1 reply; 8+ messages in thread
From: Florian Westphal @ 2017-05-19 11:36 UTC (permalink / raw)
To: Oliver Ford; +Cc: Florian Westphal, netfilter-devel
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 --
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3 1/1] iptables: Fix crash on malformed iptables-restore
2017-05-19 11:36 ` Florian Westphal
@ 2017-05-19 12:03 ` Oliver Ford
0 siblings, 0 replies; 8+ messages in thread
From: Oliver Ford @ 2017-05-19 12:03 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-05-19 12:03 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-19 9:54 [PATCH v3 1/1] iptables: Fix crash on malformed iptables-restore Oliver Ford
2017-05-19 10:04 ` Florian Westphal
2017-05-19 10:12 ` Oliver Ford
2017-05-19 10:38 ` Florian Westphal
2017-05-19 10:41 ` Oliver Ford
2017-05-19 11:24 ` Oliver Ford
2017-05-19 11:36 ` Florian Westphal
2017-05-19 12:03 ` Oliver Ford
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.