* [PATCH] iptables: improve chain name validation
@ 2013-10-05 16:33 Phil Oester
2013-10-07 14:14 ` Jan Engelhardt
2013-11-03 20:23 ` Pablo Neira Ayuso
0 siblings, 2 replies; 5+ messages in thread
From: Phil Oester @ 2013-10-05 16:33 UTC (permalink / raw)
To: netfilter-devel; +Cc: pablo
[-- Attachment #1: Type: text/plain, Size: 371 bytes --]
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>
[-- Attachment #2: patch-iptables-parse_chain --]
[-- Type: text/plain, Size: 2041 bytes --]
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. "
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] iptables: improve chain name validation
2013-10-05 16:33 [PATCH] iptables: improve chain name validation Phil Oester
@ 2013-10-07 14:14 ` Jan Engelhardt
2013-10-08 16:08 ` Phil Oester
2013-11-03 20:23 ` Pablo Neira Ayuso
1 sibling, 1 reply; 5+ messages in thread
From: Jan Engelhardt @ 2013-10-07 14:14 UTC (permalink / raw)
To: Phil Oester; +Cc: netfilter-devel, pablo
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iptables: improve chain name validation
2013-10-07 14:14 ` Jan Engelhardt
@ 2013-10-08 16:08 ` Phil Oester
0 siblings, 0 replies; 5+ messages in thread
From: Phil Oester @ 2013-10-08 16:08 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: netfilter-devel, pablo
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iptables: improve chain name validation
2013-10-05 16:33 [PATCH] iptables: improve chain name validation Phil Oester
2013-10-07 14:14 ` Jan Engelhardt
@ 2013-11-03 20:23 ` Pablo Neira Ayuso
2013-11-03 22:33 ` Phil Oester
1 sibling, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2013-11-03 20:23 UTC (permalink / raw)
To: Phil Oester; +Cc: netfilter-devel
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-11-03 22:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-05 16:33 [PATCH] iptables: improve chain name validation Phil Oester
2013-10-07 14:14 ` Jan Engelhardt
2013-10-08 16:08 ` Phil Oester
2013-11-03 20:23 ` Pablo Neira Ayuso
2013-11-03 22:33 ` Phil Oester
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).