iptables-restore cleanup & input checks

PATCH New Olaf Rempel 1 participant 1 message Nov 5, 2005 netfilter

lore

Share & tools

lore

Olaf Rempel

author

2005-11-05 22:13 UTC #0 lore raw

PATCH 1/1 iptables-restore cleanup & input checks

Hi list!

iptables-restore can restore wrong policy counters or even segfault when reading invalid/incomplete data:

# Generated by iptables-save v1.3.3 on Sat Nov 5 21:12:41 2005 *mangle

# wrong data will be inserted (return value of parse_counters() is not checked)
:PREROUTING ACCEPT [ 14226472:14821988667]

# wrong data will be inserted (like PREROUTING)
:INPUT ACCEPT [587357:]

# restore will segfault, b/c parse_counters() string-parameter is a NULL-pointer
:FORWARD ACCEPT

:OUTPUT ACCEPT [11628546:19902662543]
:POSTROUTING ACCEPT [22035607:20460618489]
COMMIT

two patches attached:
- iptables-1.3.4-exit_error.patch
some cleanup work: removes all exit() calls after exit_error() calls

- iptables-1.3.4-policycounters.patch
adds NULL-pointer check in parse_counters() and a error msg if it fails (this is similar to the "counters" patch from fedora and others, but nobody seems to care pushing :(

cheers
Olaf Rempel

ip6tables-restore.c +7 ~~-15~~

diff -uNr iptables-1.3.4.org/ip6tables-restore.c iptables-1.3.4/ip6tables-restore.c
@@ -66,11 +66,10 @@
 		handle = ip6tc_init(tablename);
 	}
 
-	if (!handle) {
+	if (!handle)
 		exit_error(PARAMETER_PROBLEM, "%s: unable to initialize"
 			"table '%s'\n", program_name, tablename);
-		exit(1);
-	}
+
 	return handle;
 }

@@ -191,12 +190,11 @@
 
 			table = strtok(buffer+1, " \t\n");
 			DEBUGP("line %u, table '%s'\n", line, table);
-			if (!table) {
+			if (!table)
 				exit_error(PARAMETER_PROBLEM, 
 					"%s: line %u table name invalid\n",
 					program_name, line);
-				exit(1);
-			}
+
 			strncpy(curtable, table, IP6T_TABLE_MAXNAMELEN);
 			curtable[IP6T_TABLE_MAXNAMELEN] = '\0';

@@ -225,12 +223,10 @@
 
 			chain = strtok(buffer+1, " \t\n");
 			DEBUGP("line %u, chain '%s'\n", line, chain);
-			if (!chain) {
+			if (!chain)
 				exit_error(PARAMETER_PROBLEM,
 					   "%s: line %u chain name invalid\n",
 					   program_name, line);
-				exit(1);
-			}
 
 			if (ip6tc_builtin(chain, handle) <= 0) {
 				if (noflush && ip6tc_is_chain(chain, handle)) {

@@ -252,12 +248,10 @@
 
 			policy = strtok(NULL, " \t\n");
 			DEBUGP("line %u, policy '%s'\n", line, policy);
-			if (!policy) {
+			if (!policy)
 				exit_error(PARAMETER_PROBLEM,
 					   "%s: line %u policy invalid\n",
 					   program_name, line);
-				exit(1);
-			}
 
 			if (strcmp(policy, "-") != 0) {
 				struct ip6t_counters count;

@@ -381,12 +375,10 @@
 
 					/* check if table name specified */
 					if (!strncmp(param_buffer, "-t", 3)
-                                            || !strncmp(param_buffer, "--table", 8)) {
+                                            || !strncmp(param_buffer, "--table", 8))
 						exit_error(PARAMETER_PROBLEM, 
 						   "Line %u seems to have a "
 						   "-t table option.\n", line);
-						exit(1);
-					}
 
 					add_argv(param_buffer);
 					param_start += param_len + 1;

iptables-restore.c +7 ~~-15~~

diff -uNr iptables-1.3.4.org/iptables-restore.c iptables-1.3.4/iptables-restore.c
@@ -63,11 +63,10 @@
 		handle = iptc_init(tablename);
 	}
 
-	if (!handle) {
+	if (!handle)
 		exit_error(PARAMETER_PROBLEM, "%s: unable to initialize"
 			"table '%s'\n", program_name, tablename);
-		exit(1);
-	}
+
 	return handle;
 }

@@ -194,12 +193,11 @@
 
 			table = strtok(buffer+1, " \t\n");
 			DEBUGP("line %u, table '%s'\n", line, table);
-			if (!table) {
+			if (!table)
 				exit_error(PARAMETER_PROBLEM, 
 					"%s: line %u table name invalid\n",
 					program_name, line);
-				exit(1);
-			}
+
 			strncpy(curtable, table, IPT_TABLE_MAXNAMELEN);
 			curtable[IPT_TABLE_MAXNAMELEN] = '\0';

@@ -228,12 +226,10 @@
 
 			chain = strtok(buffer+1, " \t\n");
 			DEBUGP("line %u, chain '%s'\n", line, chain);
-			if (!chain) {
+			if (!chain)
 				exit_error(PARAMETER_PROBLEM,
 					   "%s: line %u chain name invalid\n",
 					   program_name, line);
-				exit(1);
-			}
 
 			if (iptc_builtin(chain, handle) <= 0) {
 				if (noflush && iptc_is_chain(chain, handle)) {

@@ -255,12 +251,10 @@
 
 			policy = strtok(NULL, " \t\n");
 			DEBUGP("line %u, policy '%s'\n", line, policy);
-			if (!policy) {
+			if (!policy)
 				exit_error(PARAMETER_PROBLEM,
 					   "%s: line %u policy invalid\n",
 					   program_name, line);
-				exit(1);
-			}
 
 			if (strcmp(policy, "-") != 0) {
 				struct ipt_counters count;

@@ -384,12 +378,10 @@
 
 					/* check if table name specified */
 					if (!strncmp(param_buffer, "-t", 3)
-                                            || !strncmp(param_buffer, "--table", 8)) {
+                                            || !strncmp(param_buffer, "--table", 8))
 						exit_error(PARAMETER_PROBLEM, 
 						   "Line %u seems to have a "
 						   "-t table option.\n", line);
-						exit(1);
-					}
 
 					add_argv(param_buffer);
 					param_start += param_len + 1;

ip6tables-restore.c +8 -3

diff -uNr iptables-1.3.4.org/ip6tables-restore.c iptables-1.3.4/ip6tables-restore.c
@@ -75,7 +75,10 @@
 
 int parse_counters(char *string, struct ip6t_counters *ctr)
 {
-	return (sscanf(string, "[%llu:%llu]", (unsigned long long *)&ctr->pcnt, (unsigned long long *)&ctr->bcnt) == 2);
+	if (string)	
+		return (sscanf(string, "[%llu:%llu]", (unsigned long long *)&ctr->pcnt, (unsigned long long *)&ctr->bcnt) == 2);
+	else
+		return 0;
 }
 
 /* global new argv and argc */

@@ -260,8 +263,10 @@
 					char *ctrs;
 					ctrs = strtok(NULL, " \t\n");
 
-					parse_counters(ctrs, &count);
-
+					if (!parse_counters(ctrs, &count))
+						exit_error(PARAMETER_PROBLEM,
+							   "%s: line %u policy counters invalid\n",
+							   program_name, line);
 				} else {
 					memset(&count, 0, 
 					       sizeof(struct ip6t_counters));

iptables-restore.c +8 -3

diff -uNr iptables-1.3.4.org/iptables-restore.c iptables-1.3.4/iptables-restore.c
@@ -72,7 +72,10 @@
 
 int parse_counters(char *string, struct ipt_counters *ctr)
 {
-	return (sscanf(string, "[%llu:%llu]", (unsigned long long *)&ctr->pcnt, (unsigned long long *)&ctr->bcnt) == 2);
+	if (string)
+		return (sscanf(string, "[%llu:%llu]", (unsigned long long *)&ctr->pcnt, (unsigned long long *)&ctr->bcnt) == 2);
+	else
+		return 0;
 }
 
 /* global new argv and argc */

@@ -263,8 +266,10 @@
 					char *ctrs;
 					ctrs = strtok(NULL, " \t\n");
 
-					parse_counters(ctrs, &count);
-
+					if (!parse_counters(ctrs, &count))
+						exit_error(PARAMETER_PROBLEM,
+							   "%s: line %u policy counters invalid\n",
+							   program_name, line);
 				} else {
 					memset(&count, 0, 
 					       sizeof(struct ipt_counters));