hi,
while fuzzing iptables-restore input with afl [0],
i found a very old and known crash to be still existent,
there was even a mailing list discussion [1][2] about it
instead of fixing the real cause,
the restore input was parsed for "-t" and "--table",
however this was not enough and the error could
still be triggered by e.g. "-vtnew"
please consider/review my two attached patches
the first patch is fixing the segfault less intrusively
and the second one removes the insufficient "-t" check
[0] http://lcamtuf.coredump.cx/afl/
[1] http://lists.netfilter.org/pipermail/netfilter-devel/2001-September/005638.html
[2] http://lists.netfilter.org/pipermail/netfilter-devel/2001-October/005840.html
best regards
felix
From: Felix Bolte <bolte.felix@xxxxxxxxx>
Date: Tue, 21 Jul 2015 01:39:28 +0200
Subject: [PATCH 1/2] ip(6)tables-restore: fix segfault
The argument "table" in do_command(4|6) might be assigned to a static allocated string,
thus free_argv will crash when freeing the pointer "newargv[2]".
Fixed less intrusively by copying the content of "table" into a local variable.
Signed-off-by: Felix Bolte <bolte.felix@xxxxxxxxx>
---
iptables/ip6tables.c | 11 +++++++----
iptables/iptables.c | 13 ++++++++-----
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index 8db13b4..587f7d9 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -1339,6 +1339,9 @@ int do_command6(int argc, char *argv[], char **table,
cs.jumpto = "";
cs.argv = argv;
+ char *tmp_table = strdup(*table);
+ char **target_table = &tmp_table;
+
/* re-set optind to 0 in case do_command6 gets called
* a second time */
optind = 0;
@@ -1628,7 +1631,7 @@ int do_command6(int argc, char *argv[], char **table,
if (cs.invert)
xtables_error(PARAMETER_PROBLEM,
"unexpected ! flag before --table");
- *table = optarg;
+ *target_table = optarg;
break;
case 'x':
@@ -1774,16 +1777,16 @@ int do_command6(int argc, char *argv[], char **table,
/* only allocate handle if we weren't called with a handle */
if (!*handle)
- *handle = ip6tc_init(*table);
+ *handle = ip6tc_init(*target_table);
/* try to insmod the module if iptc_init failed */
if (!*handle && xtables_load_ko(xtables_modprobe_program, false) != -1)
- *handle = ip6tc_init(*table);
+ *handle = ip6tc_init(*target_table);
if (!*handle)
xtables_error(VERSION_PROBLEM,
"can't initialize ip6tables table `%s': %s",
- *table, ip6tc_strerror(errno));
+ *target_table, ip6tc_strerror(errno));
if (command == CMD_APPEND
|| command == CMD_DELETE
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 88953c4..98e7d34 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -1335,6 +1335,9 @@ int do_command4(int argc, char *argv[], char **table,
cs.jumpto = "";
cs.argv = argv;
+ char *tmp_table = strdup(*table);
+ char **target_table = &tmp_table;
+
/* re-set optind to 0 in case do_command4 gets called
* a second time */
optind = 0;
@@ -1621,7 +1624,7 @@ int do_command4(int argc, char *argv[], char **table,
if (cs.invert)
xtables_error(PARAMETER_PROBLEM,
"unexpected ! flag before --table");
- *table = optarg;
+ *target_table = optarg;
break;
case 'x':
@@ -1708,7 +1711,7 @@ int do_command4(int argc, char *argv[], char **table,
cs.invert = FALSE;
}
- if (strcmp(*table, "nat") == 0 &&
+ if (strcmp(*target_table, "nat") == 0 &&
((policy != NULL && strcmp(policy, "DROP") == 0) ||
(cs.jumpto != NULL && strcmp(cs.jumpto, "DROP") == 0)))
xtables_error(PARAMETER_PROBLEM,
@@ -1770,16 +1773,16 @@ int do_command4(int argc, char *argv[], char **table,
/* only allocate handle if we weren't called with a handle */
if (!*handle)
- *handle = iptc_init(*table);
+ *handle = iptc_init(*target_table);
/* try to insmod the module if iptc_init failed */
if (!*handle && xtables_load_ko(xtables_modprobe_program, false) != -1)
- *handle = iptc_init(*table);
+ *handle = iptc_init(*target_table);
if (!*handle)
xtables_error(VERSION_PROBLEM,
"can't initialize iptables table `%s': %s",
- *table, iptc_strerror(errno));
+ *target_table, iptc_strerror(errno));
if (command == CMD_APPEND
|| command == CMD_DELETE
--
1.7.9.5
From: Felix Bolte <bolte.felix@xxxxxxxxx>
Date: Tue, 21 Jul 2015 15:36:18 +0200
Subject: [PATCH 2/2] ip(6)tables-restore: remove old "-t" check
This check became obsolete as the follow up segfault has been fixed.
Furthermore the check was insufficient as "-vt" was still triggering an error.
We should let iptables decide whether the passed arguments are valid or not.
However, multiple "-t" arguments should not harm the outcome as the handle dictates the current table.
Signed-off-by: Felix Bolte <bolte.felix@xxxxxxxxx>
---
iptables/ip6tables-restore.c | 9 ---------
iptables/iptables-restore.c | 9 ---------
2 files changed, 18 deletions(-)
diff --git a/iptables/ip6tables-restore.c b/iptables/ip6tables-restore.c
index 9393924..cc07b7c 100644
--- a/iptables/ip6tables-restore.c
+++ b/iptables/ip6tables-restore.c
@@ -154,15 +154,6 @@ 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)) {
- xtables_error(PARAMETER_PROBLEM,
- "The -t option (seen in line %u) cannot be "
- "used in ip6tables-restore.\n", line);
- exit(1);
- }
-
add_argv(param_buffer);
param_len = 0;
} else {
diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index 638b171..488edb9 100644
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -153,15 +153,6 @@ 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)) {
- xtables_error(PARAMETER_PROBLEM,
- "The -t option (seen in line %u) cannot be "
- "used in iptables-restore.\n", line);
- exit(1);
- }
-
add_argv(param_buffer);
param_len = 0;
} else {
--
1.7.9.5
