ip(6)tables-restore segfault + patch

ip(6)tables-restore segfault + patch

[Felix Bolte]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

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

[-- Attachment #2: 0001-ip-6-tables-restore-fix-segfault.patch --]
[-- Type: text/x-patch, Size: 3628 bytes --]

From: Felix Bolte <bolte.felix@gmail.com>
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@gmail.com>
---
 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


[-- Attachment #3: 0002-ip-6-tables-restore-remove-old-t-check.patch --]
[-- Type: text/x-patch, Size: 1939 bytes --]

From: Felix Bolte <bolte.felix@gmail.com>
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@gmail.com>
---
 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