* [PATCH v2 1/1] iptables: Fix crash on malformed iptables-restore
@ 2017-05-18 14:18 Oliver Ford
2017-05-18 14:42 ` Florian Westphal
0 siblings, 1 reply; 5+ messages in thread
From: Oliver Ford @ 2017-05-18 14:18 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 '--' followed by a 't' anywhere in the parameter where the
parameter is not in a list of valid iptables options (the list is every iptables
option that contains a 't' except for the 'table' option). This filters parameters
like '--tab' but allows valid ones like '--protocol'.
Signed-off-by: Oliver Ford <ojford@gmail.com>
---
iptables/ip6tables-restore.c | 27 +++++++++++++++++++++++----
iptables/iptables-restore.c | 27 +++++++++++++++++++++++----
iptables/iptables-xml.c | 29 +++++++++++++++++++++++------
iptables/xtables-restore.c | 27 +++++++++++++++++++++++----
4 files changed, 92 insertions(+), 18 deletions(-)
diff --git a/iptables/ip6tables-restore.c b/iptables/ip6tables-restore.c
index 39a881d..0c6f2e4 100644
--- a/iptables/ip6tables-restore.c
+++ b/iptables/ip6tables-restore.c
@@ -165,14 +165,33 @@ 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')) {
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);
+ } else if (!strncmp(param_buffer, "--", 2)
+ && strchr(param_buffer, 't')) {
+ /* If we begin with a '--' and have a 't', check
+ * that the parameter is in the list of valid options */
+ const char* t_options[] = {
+ "delete", "insert", "list", "list-rules", "delete-chain",
+ "destination", "dst", "protocol", "in-interface", "match",
+ "out-interface", "wait", "wait-interval", "exact",
+ "fragments", "set-counters", "goto"};
+ int i, opt_len = ARRAY_SIZE(t_options);
+ for (i = 0; i < opt_len; i++) {
+ if (!strcmp(param_buffer + 2, t_options[i])) {
+ goto t_passed;
+ }
+ }
+ xtables_error(PARAMETER_PROBLEM,
+ "Invalid option in line %u: %s\n", line, param_buffer);
exit(1);
}
+ t_passed:
add_argv(param_buffer);
param_len = 0;
} else {
diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index 876fe06..4ba9be6 100644
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -162,14 +162,33 @@ 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')) {
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);
+ } else if (!strncmp(param_buffer, "--", 2)
+ && strchr(param_buffer, 't')) {
+ /* If we begin with a '--' and have a 't', check
+ * that the parameter is in the list of valid options */
+ const char* t_options[] = {
+ "delete", "insert", "list", "list-rules", "delete-chain",
+ "destination", "dst", "protocol", "in-interface", "match",
+ "out-interface", "wait", "wait-interval", "exact",
+ "fragments", "set-counters", "goto"};
+ int i, opt_len = ARRAY_SIZE(t_options);
+ for (i = 0; i < opt_len; i++) {
+ if (!strcmp(param_buffer + 2, t_options[i])) {
+ goto t_passed;
+ }
+ }
+ xtables_error(PARAMETER_PROBLEM,
+ "Invalid option in line %u: %s\n", line, param_buffer);
exit(1);
}
+ t_passed:
add_argv(param_buffer);
param_len = 0;
} else {
diff --git a/iptables/iptables-xml.c b/iptables/iptables-xml.c
index 2e093b5..d93ca56 100644
--- a/iptables/iptables-xml.c
+++ b/iptables/iptables-xml.c
@@ -819,16 +819,33 @@ 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')) {
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);
+ } else if (!strncmp(param_buffer, "--", 2)
+ && strchr(param_buffer, 't')) {
+ /* If we begin with a '--' and have a 't', check
+ * that the parameter is in the list of valid options */
+ const char* t_options[] = {
+ "delete", "insert", "list", "list-rules", "delete-chain",
+ "destination", "dst", "protocol", "in-interface", "match",
+ "out-interface", "wait", "wait-interval", "exact",
+ "fragments", "set-counters", "goto"};
+ int i, opt_len = ARRAY_SIZE(t_options);
+ for (i = 0; i < opt_len; i++) {
+ if (!strcmp(param_buffer + 2, t_options[i])) {
+ goto t_passed;
+ }
+ }
+ xtables_error(PARAMETER_PROBLEM,
+ "Invalid option in line %u: %s\n", line, param_buffer);
exit(1);
}
+ t_passed:
add_argv(param_buffer, quoted);
if (newargc >= 2
&& 0 ==
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 15824f0..357f9b0 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -136,14 +136,33 @@ 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')) {
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);
+ } else if (!strncmp(param_buffer, "--", 2)
+ && strchr(param_buffer, 't')) {
+ /* If we begin with a '--' and have a 't', check
+ * that the parameter is in the list of valid options */
+ const char* t_options[] = {
+ "delete", "insert", "list", "list-rules", "delete-chain",
+ "destination", "dst", "protocol", "in-interface", "match",
+ "out-interface", "wait", "wait-interval", "exact",
+ "fragments", "set-counters", "goto"};
+ int i, opt_len = ARRAY_SIZE(t_options);
+ for (i = 0; i < opt_len; i++) {
+ if (!strcmp(param_buffer + 2, t_options[i])) {
+ goto t_passed;
+ }
+ }
+ xtables_error(PARAMETER_PROBLEM,
+ "Invalid option in line %u: %s\n", line, param_buffer);
exit(1);
}
+ t_passed:
add_argv(param_buffer);
param_len = 0;
} else {
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] iptables: Fix crash on malformed iptables-restore
2017-05-18 14:18 [PATCH v2 1/1] iptables: Fix crash on malformed iptables-restore Oliver Ford
@ 2017-05-18 14:42 ` Florian Westphal
[not found] ` <CAGMVOdswbPCFa1Di6FGGBSznkz36E+iN4b0QJFyS1Ex7JYr75w@mail.gmail.com>
0 siblings, 1 reply; 5+ messages in thread
From: Florian Westphal @ 2017-05-18 14:42 UTC (permalink / raw)
To: Oliver Ford; +Cc: netfilter-devel
Oliver Ford <ojford@gmail.com> wrote:
> --- a/iptables/ip6tables-restore.c
> +++ b/iptables/ip6tables-restore.c
> @@ -165,14 +165,33 @@ 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')) {
> 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);
> + } else if (!strncmp(param_buffer, "--", 2)
> + && strchr(param_buffer, 't')) {
Why this strchr() ?
if (!strncmp(param_buffer, "--t", 3) &&
!strncmp(param_buffer, "--table", strlen(param_buffer))
err();
should work.
> + /* If we begin with a '--' and have a 't', check
> + * that the parameter is in the list of valid options */
> + const char* t_options[] = {
If this is needed, I'd suggest
static const char * const t_options[] = {
> + "delete", "insert", "list", "list-rules", "delete-chain",
> + "destination", "dst", "protocol", "in-interface", "match",
> + "out-interface", "wait", "wait-interval", "exact",
> + "fragments", "set-counters", "goto"};
> + int i, opt_len = ARRAY_SIZE(t_options);
> + for (i = 0; i < opt_len; i++) {
> + if (!strcmp(param_buffer + 2, t_options[i])) {
> + goto t_passed;
> + }
> + }
If this t_options[] thing is really needed i'd try to stick this into
a helper function so we don't have to duplicate this in all 3
incarnations.
Thanks for working on this.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] iptables: Fix crash on malformed iptables-restore
@ 2017-05-18 15:21 Oliver Ford
0 siblings, 0 replies; 5+ messages in thread
From: Oliver Ford @ 2017-05-18 15:21 UTC (permalink / raw)
To: netfilter-devel
On Thu, May 18, 2017 at 3:42 PM, Florian Westphal <fw@strlen.de> wrote:
> Oliver Ford <ojford@gmail.com> wrote:
>> --- a/iptables/ip6tables-restore.c
>> +++ b/iptables/ip6tables-restore.c
>> @@ -165,14 +165,33 @@ 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')) {
>> 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);
>> + } else if (!strncmp(param_buffer, "--", 2)
>> + && strchr(param_buffer, 't')) {
>
> Why this strchr() ?
>
> if (!strncmp(param_buffer, "--t", 3) &&
> !strncmp(param_buffer, "--table", strlen(param_buffer))
> err();
>
> should work.
>
"strncmp(param_buffer, "--t", 3)" wouldn't return 0 if you had an
invalid parameter like "--ftabl".
>> + /* If we begin with a '--' and have a 't', check
>> + * that the parameter is in the list of valid options */
>> + const char* t_options[] = {
>
> If this is needed, I'd suggest
>
> static const char * const t_options[] = {
>
>> + "delete", "insert", "list", "list-rules", "delete-chain",
>> + "destination", "dst", "protocol", "in-interface", "match",
>> + "out-interface", "wait", "wait-interval", "exact",
>> + "fragments", "set-counters", "goto"};
>> + int i, opt_len = ARRAY_SIZE(t_options);
>> + for (i = 0; i < opt_len; i++) {
>> + if (!strcmp(param_buffer + 2, t_options[i])) {
>> + goto t_passed;
>> + }
>> + }
>
> If this t_options[] thing is really needed i'd try to stick this into
> a helper function so we don't have to duplicate this in all 3
> incarnations.
>
> Thanks for working on this.
Can do, but the code already duplicates everything else across all 3
incarnations so I thought it best to stick with the established
pattern :-) If you'd prefer it extracted I'll send a new version.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] iptables: Fix crash on malformed iptables-restore
[not found] ` <20170518152003.GD1272@breakpoint.cc>
@ 2017-05-18 15:29 ` Oliver Ford
2017-05-19 9:55 ` Oliver Ford
0 siblings, 1 reply; 5+ messages in thread
From: Oliver Ford @ 2017-05-18 15:29 UTC (permalink / raw)
To: Florian Westphal, netfilter-devel
On Thu, May 18, 2017 at 4:20 PM, Florian Westphal <fw@strlen.de> wrote:
> Oliver Ford <ojford@gmail.com> wrote:
>> On Thu, May 18, 2017 at 3:42 PM, Florian Westphal <fw@strlen.de> wrote:
>> > Oliver Ford <ojford@gmail.com> wrote:
>> >> --- a/iptables/ip6tables-restore.c
>> >> +++ b/iptables/ip6tables-restore.c
>> >> @@ -165,14 +165,33 @@ 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')) {
>> >> 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);
>> >> + } else if (!strncmp(param_buffer, "--", 2)
>> >> + && strchr(param_buffer, 't')) {
>> >
>> > Why this strchr() ?
>> >
>> > if (!strncmp(param_buffer, "--t", 3) &&
>> > !strncmp(param_buffer, "--table", strlen(param_buffer))
>> > err();
>> >
>> > should work.
>> >
>>
>> "strncmp(param_buffer, "--t", 3)" wouldn't return 0 if you had an
>> invalid parameter like "--ftabl".
>
> Yes, but that will just throw an error:
> iptables-restore v1.6.0: unknown option "--ftab"
>
>> >> + /* If we begin with a '--' and have a 't', check
>> >> + * that the parameter is in the list of valid options */
>> >> + const char* t_options[] = {
>> >
>> > If this is needed, I'd suggest
>> >
>> > static const char * const t_options[] = {
>> >
>> >> + "delete", "insert", "list", "list-rules", "delete-chain",
>> >> + "destination", "dst", "protocol", "in-interface", "match",
>> >> + "out-interface", "wait", "wait-interval", "exact",
>> >> + "fragments", "set-counters", "goto"};
>> >> + int i, opt_len = ARRAY_SIZE(t_options);
>> >> + for (i = 0; i < opt_len; i++) {
>> >> + if (!strcmp(param_buffer + 2, t_options[i])) {
>> >> + goto t_passed;
>> >> + }
>> >> + }
>> >
>> > If this t_options[] thing is really needed i'd try to stick this into
>> > a helper function so we don't have to duplicate this in all 3
>> > incarnations.
>> >
>> > Thanks for working on this.
>>
>> Can do, but the code already duplicates everything else across all 3
>> incarnations so I thought it best to stick with the established
>> pattern :-) If you'd prefer it extracted I'll send a new version.
>
> That pattern sucks :-)
Ok, I'll make version three with these changes.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] iptables: Fix crash on malformed iptables-restore
2017-05-18 15:29 ` Oliver Ford
@ 2017-05-19 9:55 ` Oliver Ford
0 siblings, 0 replies; 5+ messages in thread
From: Oliver Ford @ 2017-05-19 9:55 UTC (permalink / raw)
To: Florian Westphal, netfilter-devel
On Thu, May 18, 2017 at 4:29 PM, Oliver Ford <ojford@gmail.com> wrote:
> On Thu, May 18, 2017 at 4:20 PM, Florian Westphal <fw@strlen.de> wrote:
>> Oliver Ford <ojford@gmail.com> wrote:
>>> On Thu, May 18, 2017 at 3:42 PM, Florian Westphal <fw@strlen.de> wrote:
>>> > Oliver Ford <ojford@gmail.com> wrote:
>>> >> --- a/iptables/ip6tables-restore.c
>>> >> +++ b/iptables/ip6tables-restore.c
>>> >> @@ -165,14 +165,33 @@ 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')) {
>>> >> 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);
>>> >> + } else if (!strncmp(param_buffer, "--", 2)
>>> >> + && strchr(param_buffer, 't')) {
>>> >
>>> > Why this strchr() ?
>>> >
>>> > if (!strncmp(param_buffer, "--t", 3) &&
>>> > !strncmp(param_buffer, "--table", strlen(param_buffer))
>>> > err();
>>> >
>>> > should work.
>>> >
>>>
>>> "strncmp(param_buffer, "--t", 3)" wouldn't return 0 if you had an
>>> invalid parameter like "--ftabl".
>>
>> Yes, but that will just throw an error:
>> iptables-restore v1.6.0: unknown option "--ftab"
>>
>>> >> + /* If we begin with a '--' and have a 't', check
>>> >> + * that the parameter is in the list of valid options */
>>> >> + const char* t_options[] = {
>>> >
>>> > If this is needed, I'd suggest
>>> >
>>> > static const char * const t_options[] = {
>>> >
>>> >> + "delete", "insert", "list", "list-rules", "delete-chain",
>>> >> + "destination", "dst", "protocol", "in-interface", "match",
>>> >> + "out-interface", "wait", "wait-interval", "exact",
>>> >> + "fragments", "set-counters", "goto"};
>>> >> + int i, opt_len = ARRAY_SIZE(t_options);
>>> >> + for (i = 0; i < opt_len; i++) {
>>> >> + if (!strcmp(param_buffer + 2, t_options[i])) {
>>> >> + goto t_passed;
>>> >> + }
>>> >> + }
>>> >
>>> > If this t_options[] thing is really needed i'd try to stick this into
>>> > a helper function so we don't have to duplicate this in all 3
>>> > incarnations.
>>> >
>>> > Thanks for working on this.
>>>
>>> Can do, but the code already duplicates everything else across all 3
>>> incarnations so I thought it best to stick with the established
>>> pattern :-) If you'd prefer it extracted I'll send a new version.
>>
>> That pattern sucks :-)
>
> Ok, I'll make version three with these changes.
I've just sent version 3 with a much simplified check. Now it filters
a single '-' with a 't' anywhere, or a '--t'. That seems to do the
trick.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-05-19 9:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-18 14:18 [PATCH v2 1/1] iptables: Fix crash on malformed iptables-restore Oliver Ford
2017-05-18 14:42 ` Florian Westphal
[not found] ` <CAGMVOdswbPCFa1Di6FGGBSznkz36E+iN4b0QJFyS1Ex7JYr75w@mail.gmail.com>
[not found] ` <20170518152003.GD1272@breakpoint.cc>
2017-05-18 15:29 ` Oliver Ford
2017-05-19 9:55 ` Oliver Ford
-- strict thread matches above, loose matches on Subject: below --
2017-05-18 15:21 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.