diff mbox

iptables: improve chain name validation

Message ID 20131005163315.GA16881@home
State Accepted
Headers show

Commit Message

Phil Oester Oct. 5, 2013, 4:33 p.m. UTC 
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>

Comments

Jan Engelhardt Oct. 7, 2013, 2:14 p.m. UTC | #1 
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
Phil Oester Oct. 8, 2013, 4:08 p.m. UTC | #2 
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
Pablo Neira Ayuso Nov. 3, 2013, 8:23 p.m. UTC | #3 
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
Phil Oester Nov. 3, 2013, 10:33 p.m. UTC | #4 
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 mbox

Patch

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. "