Message ID | 20131005163315.GA16881@home |
---|---|
State | Accepted |
Headers | show |
On Saturday 2013-10-05 18:33, Phil Oester wrote: >As pointed out by Andrew Domaszek, iptables allows whitespace to be >included in chain names. This causes issues with iptables-restore, and >later iptables actions on the chain. Attached patch disallows >whitespace, and also consolidates all chain name checking into a new >function. IMO iptables-save should just put the chain name in quotes (calling xtables_save_string) if there is a need to do so. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 07, 2013 at 04:14:47PM +0200, Jan Engelhardt wrote: > IMO iptables-save should just put the chain name in quotes (calling > xtables_save_string) if there is a need to do so. Example: # iptables -N 'hog wash' # iptables -A INPUT -j 'hog wash' iptables v1.4.20: Invalid target name `hog wash' Try `iptables -h' or 'iptables --help' for more information. Code: static const char * parse_target(const char *targetname) { ... for (ptr = targetname; *ptr; ptr++) if (isspace(*ptr)) xtables_error(PARAMETER_PROBLEM, "Invalid target name `%s'", targetname); Conclusion: It seems pointless to allow adding chains which cannot actually be used. Phil -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Oct 05, 2013 at 09:33:15AM -0700, Phil Oester wrote: > As pointed out by Andrew Domaszek, iptables allows whitespace to be included in > chain names. This causes issues with iptables-restore, and later iptables > actions on the chain. Attached patch disallows whitespace, and also consolidates > all chain name checking into a new function. > > This closes netfilter bugzilla #855. Applied, thanks Phil. I have mangled this patch to include the ip6tables.c chunk, let me know if you find any problem with it. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Nov 03, 2013 at 09:23:23PM +0100, Pablo Neira Ayuso wrote: > Applied, thanks Phil. > > I have mangled this patch to include the ip6tables.c chunk, let me > know if you find any problem with it. Looks good. Sorry I missed the -6 version. Phil -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/iptables/iptables.c b/iptables/iptables.c index d3899bc..5cd2596 100644 --- a/iptables/iptables.c +++ b/iptables/iptables.c @@ -373,6 +373,32 @@ parse_rulenumber(const char *rule) return rulenum; } +static void +parse_chain(const char *chainname) +{ + const char *ptr; + + if (strlen(chainname) >= XT_EXTENSION_MAXNAMELEN) + xtables_error(PARAMETER_PROBLEM, + "chain name `%s' too long (must be under %u chars)", + chainname, XT_EXTENSION_MAXNAMELEN); + + if (*chainname == '-' || *chainname == '!') + xtables_error(PARAMETER_PROBLEM, + "chain name not allowed to start " + "with `%c'\n", *chainname); + + if (xtables_find_target(chainname, XTF_TRY_LOAD)) + xtables_error(PARAMETER_PROBLEM, + "chain name may not clash " + "with target name\n"); + + for (ptr = chainname; *ptr; ptr++) + if (isspace(*ptr)) + xtables_error(PARAMETER_PROBLEM, + "Invalid chain name `%s'", chainname); +} + static const char * parse_target(const char *targetname) { @@ -1428,14 +1454,7 @@ int do_command4(int argc, char *argv[], char **table, break; case 'N': - if (optarg && (*optarg == '-' || *optarg == '!')) - xtables_error(PARAMETER_PROBLEM, - "chain name not allowed to start " - "with `%c'\n", *optarg); - if (xtables_find_target(optarg, XTF_TRY_LOAD)) - xtables_error(PARAMETER_PROBLEM, - "chain name may not clash " - "with target name\n"); + parse_chain(optarg); add_command(&command, CMD_NEW_CHAIN, CMD_NONE, cs.invert); chain = optarg; @@ -1729,11 +1748,6 @@ int do_command4(int argc, char *argv[], char **table, generic_opt_check(command, cs.options); - if (chain != NULL && strlen(chain) >= XT_EXTENSION_MAXNAMELEN) - xtables_error(PARAMETER_PROBLEM, - "chain name `%s' too long (must be under %u chars)", - chain, XT_EXTENSION_MAXNAMELEN); - /* Attempt to acquire the xtables lock */ if (!restore && !xtables_lock(wait)) { fprintf(stderr, "Another app is currently holding the xtables lock. "
As pointed out by Andrew Domaszek, iptables allows whitespace to be included in chain names. This causes issues with iptables-restore, and later iptables actions on the chain. Attached patch disallows whitespace, and also consolidates all chain name checking into a new function. This closes netfilter bugzilla #855. Signed-off-by: Phil Oester <kernel@linuxace.com>