From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olaf Rempel Subject: [PATCH] iptables-restore cleanup & input checks Date: Sat, 5 Nov 2005 23:13:10 +0100 Message-ID: <20051105231310.2fa970ab@coruscant.lan> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="Multipart_Sat__5_Nov_2005_23_13_10_+0100_BD/TyNtwEHh6sfrG" Return-path: To: netfilter-devel@lists.netfilter.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: netfilter-devel-bounces@lists.netfilter.org Errors-To: netfilter-devel-bounces@lists.netfilter.org List-Id: netfilter-devel.vger.kernel.org --Multipart_Sat__5_Nov_2005_23_13_10_+0100_BD/TyNtwEHh6sfrG Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Content-Disposition: inline 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 --Multipart_Sat__5_Nov_2005_23_13_10_+0100_BD/TyNtwEHh6sfrG Content-Type: text/x-patch; name=iptables-1.3.4-exit_error.patch Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=iptables-1.3.4-exit_error.patch diff -uNr iptables-1.3.4.org/ip6tables-restore.c iptables-1.3.4/ip6tables-restore.c --- iptables-1.3.4.org/ip6tables-restore.c 2005-06-24 18:34:19.000000000 +0200 +++ iptables-1.3.4/ip6tables-restore.c 2005-11-05 22:31:32.000000000 +0100 @@ -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; diff -uNr iptables-1.3.4.org/iptables-restore.c iptables-1.3.4/iptables-restore.c --- iptables-1.3.4.org/iptables-restore.c 2005-06-24 18:34:19.000000000 +0200 +++ iptables-1.3.4/iptables-restore.c 2005-11-05 22:32:21.000000000 +0100 @@ -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; --Multipart_Sat__5_Nov_2005_23_13_10_+0100_BD/TyNtwEHh6sfrG Content-Type: text/x-patch; name=iptables-1.3.4-policycounters.patch Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=iptables-1.3.4-policycounters.patch diff -uNr iptables-1.3.4.org/ip6tables-restore.c iptables-1.3.4/ip6tables-restore.c --- iptables-1.3.4.org/ip6tables-restore.c 2005-11-05 22:38:24.000000000 +0100 +++ iptables-1.3.4/ip6tables-restore.c 2005-11-05 22:39:52.000000000 +0100 @@ -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)); diff -uNr iptables-1.3.4.org/iptables-restore.c iptables-1.3.4/iptables-restore.c --- iptables-1.3.4.org/iptables-restore.c 2005-11-05 22:38:24.000000000 +0100 +++ iptables-1.3.4/iptables-restore.c 2005-11-05 22:36:09.000000000 +0100 @@ -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)); --Multipart_Sat__5_Nov_2005_23_13_10_+0100_BD/TyNtwEHh6sfrG--