Skip to contentSkip to conversation

iptables-save saves invalid negative --uid-owner expressions.

PATCH In review Costa Tsaousis 5 participants 11 messages Jan 6, 2003 – Jan 8, 2003 netfilter

lore
Share & tools
lore

Costa Tsaousis

author
#0 lore raw

Hi all,

I believe I have faced another, not-mentioned-in-2002, bug:

If you setup negative owner expressions like this:

# a transparent cache for the machine running the cache
iptables -t nat -A OUTPUT \
    -m owner ! --uid-owner squid \
    -p tcp --dport 80 ! -d 127.0.0.1 \
    -j DNAT --to 127.0.0.1:3128

then iptables-save gives this:

-A OUTPUT -d ! 127.0.0.1 -p tcp -m owner --uid-owner !squid -m tcp --dport 80 -j DNAT --to-destination 127.0.0.1:3128

which produces a "user '!squid' not found" error when restored.

Kernel : 2.4.18-19.8.0smp
iptables: 1.2.6a
Distro : RedHat 8.0

Regards.

Signature · 8 lines
Costa Tsaousis

Author of
FireHOL (http://firehol.sf.net)
-------------------------------
An iptables stateful firewall generator,
that allows you to configure your firewall...
...the way you think of it!
#1 lore raw

On Mon, Jan 06, 2003 at 03:31:07AM +0200, Costa Tsaousis wrote:

Hi,

Quoted 8 lines

I believe I have faced another, not-mentioned-in-2002, bug:
[...]
then iptables-save gives this:

-A OUTPUT -d ! 127.0.0.1 -p tcp -m owner --uid-owner !squid -m tcp --dport 80 -j DNAT --to-destination 127.0.0.1:3128

which produces a "user '!squid' not found" error when restored.

It seems to me that no single week passes without any post about this kind of error (no space before a negation) in some save output module. Sigh.
I must admit it seems quite strange to me that noone in the coreteam didn't do that already. (hope I won't hurt anybody's feelings too much though) ;-)

Would someone who's even more fed up than me with these recurrent messages have time to audit the whole bunch of existing modules in the CVS tree and fix this particular kind of problem _once and for all_?

Thanks in advance,

Herve

Signature · 4 lines
 _
(°=  Hervé Eychenne
//)
v_/_ WallFire project:  http://www.wallfire.org/
#2 lore raw
Quoted 10 lines · Hervé Eychenne

It seems to me that no single week passes without any post about this kind of error (no space before a negation) in some save output module. Sigh.
I must admit it seems quite strange to me that noone in the coreteam didn't do that already. (hope I won't hurt anybody's feelings too much though) ;-)

Would someone who's even more fed up than me with these recurrent messages have time to audit the whole bunch of existing modules in the CVS tree and fix this particular kind of problem _once and for all_?

All these potential bugs could be solved by using a scheme in which no new file format is introduced :) A scheme where the table is saved to a file instead of to the kernel until all modifications are done is really not that hard to do. It's being done in ebtables... With this scheme, introducing a new module keeps everything working.

Signature · 2 lines
cheers,
Bart
#3 lore raw

PATCH 1/1 : fix all iptables problems with "!" (hopefully)

In reply to Hervé Eychenne
Quoted 24 lines · Hervé Eychenne

On Mon, Jan 06, 2003 at 03:31:07AM +0200, Costa Tsaousis wrote:

Hi,

Quoted 10 lines

I believe I have faced another, not-mentioned-in-2002, bug:
[...]
then iptables-save gives this:

-A OUTPUT -d ! 127.0.0.1 -p tcp -m owner --uid-owner !squid -m tcp --dport 80 -j DNAT --to-destination 127.0.0.1:3128

which produces a "user '!squid' not found" error when restored.

It seems to me that no single week passes without any post about this kind of error (no space before a negation) in some save output module. Sigh.
I must admit it seems quite strange to me that noone in the coreteam didn't do that already. (hope I won't hurt anybody's feelings too much though) ;-)

Quoted 6 lines

Would someone who's even more fed up than me with these recurrent messages have time to audit the whole bunch of existing modules in the CVS tree and fix this particular kind of problem _once and for all_?

I did, here is the (untested) patch, hopefully i didn't miss any. I didn't change any of the "save"-functions, this patch just replaces all uses of optarg after check_inverse by argv[optind-1].

Regards,
Patrick

Quoted 6 lines

Thanks in advance,

Herve

Index: extensions/libip6t_mark.c
===================================================================
RCS file: /cvspublic/netfilter/userspace/extensions/libip6t_mark.c,v retrieving revision 1.4

libip6t_mark.c +3 -3
diff -u -r1.4 libip6t_mark.c
@@ -46,13 +46,13 @@
 		char *end;
 	case '1':
 		check_inverse(optarg, &invert, &optind, 0);
-		markinfo->mark = strtoul(optarg, &end, 0);
+		markinfo->mark = strtoul(argv[optind-1], &end, 0);
 		if (*end == '/') {
 			markinfo->mask = strtoul(end+1, &end, 0);
 		} else
 			markinfo->mask = 0xffffffff;
-		if (*end != '\0' || end == optarg)
-			exit_error(PARAMETER_PROBLEM, "Bad MARK value `%s'", optarg);
+		if (*end != '\0' || end == argv[optind-1])
+			exit_error(PARAMETER_PROBLEM, "Bad MARK value `%s'", argv[optind-1]);
 		if (invert)
 			markinfo->invert = 1;
 		*flags = 1;

Index: extensions/libip6t_owner.c
===================================================================
RCS file: /cvspublic/netfilter/userspace/extensions/libip6t_owner.c,v retrieving revision 1.4

libip6t_owner.c +14 -14
diff -u -r1.4 libip6t_owner.c
@@ -57,12 +57,12 @@
 	case '1':
 		check_inverse(optarg, &invert, &optind, 0);
 
-		if ((pwd = getpwnam(optarg)))
+		if ((pwd = getpwnam(argv[optind-1])))
 			ownerinfo->uid = pwd->pw_uid;
 		else {
-			ownerinfo->uid = strtoul(optarg, &end, 0);
-			if (*end != '\0' || end == optarg)
-				exit_error(PARAMETER_PROBLEM, "Bad OWNER UID value `%s'", optarg);
+			ownerinfo->uid = strtoul(argv[optind-1], &end, 0);
+			if (*end != '\0' || end == argv[optind-1])
+				exit_error(PARAMETER_PROBLEM, "Bad OWNER UID value `%s'", argv[optind-1]);
 		}
 		if (invert)
 			ownerinfo->invert |= IP6T_OWNER_UID;

@@ -72,12 +72,12 @@
 
 	case '2':
 		check_inverse(optarg, &invert, &optind, 0);
-		if ((grp = getgrnam(optarg)))
+		if ((grp = getgrnam(argv[optind-1])))
 			ownerinfo->gid = grp->gr_gid;
 		else {
-			ownerinfo->gid = strtoul(optarg, &end, 0);
-			if (*end != '\0' || end == optarg)
-				exit_error(PARAMETER_PROBLEM, "Bad OWNER GID value `%s'", optarg);
+			ownerinfo->gid = strtoul(argv[optind-1], &end, 0);
+			if (*end != '\0' || end == argv[optind-1])
+				exit_error(PARAMETER_PROBLEM, "Bad OWNER GID value `%s'", argv[optind-1]);
 		}
 		if (invert)
 			ownerinfo->invert |= IP6T_OWNER_GID;

@@ -87,9 +87,9 @@
 
 	case '3':
 		check_inverse(optarg, &invert, &optind, 0);
-		ownerinfo->pid = strtoul(optarg, &end, 0);
-		if (*end != '\0' || end == optarg)
-			exit_error(PARAMETER_PROBLEM, "Bad OWNER PID value `%s'", optarg);
+		ownerinfo->pid = strtoul(argv[optind-1], &end, 0);
+		if (*end != '\0' || end == argv[optind-1])
+			exit_error(PARAMETER_PROBLEM, "Bad OWNER PID value `%s'", argv[optind-1]);
 		if (invert)
 			ownerinfo->invert |= IP6T_OWNER_PID;
 		ownerinfo->match |= IP6T_OWNER_PID;

@@ -98,9 +98,9 @@
 
 	case '4':
 		check_inverse(optarg, &invert, &optind, 0);
-		ownerinfo->sid = strtoul(optarg, &end, 0);
-		if (*end != '\0' || end == optarg)
-			exit_error(PARAMETER_PROBLEM, "Bad OWNER SID value `%s'", optarg);
+		ownerinfo->sid = strtoul(argv[optind-1], &end, 0);
+		if (*end != '\0' || end == argv[optind-1])
+			exit_error(PARAMETER_PROBLEM, "Bad OWNER SID value `%s'", argv[optind-1]);
 		if (invert)
 			ownerinfo->invert |= IP6T_OWNER_SID;
 		ownerinfo->match |= IP6T_OWNER_SID;

Index: extensions/libipt_connmark.c
===================================================================
RCS file: /cvspublic/netfilter/userspace/extensions/libipt_connmark.c,v retrieving revision 1.5

libipt_connmark.c +3 -3
diff -u -r1.5 libipt_connmark.c
@@ -46,13 +46,13 @@
 		char *end;
 	case '1':
 		check_inverse(optarg, &invert, &optind, 0);
-		markinfo->mark = strtoul(optarg, &end, 0);
+		markinfo->mark = strtoul(argv[optind-1], &end, 0);
 		if (*end == '/') {
 			markinfo->mask = strtoul(end+1, &end, 0);
 		} else
 			markinfo->mask = 0xffffffff;
-		if (*end != '\0' || end == optarg)
-			exit_error(PARAMETER_PROBLEM, "Bad MARK value `%s'", optarg);
+		if (*end != '\0' || end == argv[optind-1])
+			exit_error(PARAMETER_PROBLEM, "Bad MARK value `%s'", argv[optind-1]);
 		if (invert)
 			markinfo->invert = 1;
 		*flags = 1;

Index: extensions/libipt_ecn.c
===================================================================
RCS file: /cvspublic/netfilter/userspace/extensions/libipt_ecn.c,v retrieving revision 1.3

libipt_ecn.c +1 -1
diff -u -r1.3 libipt_ecn.c
@@ -80,7 +80,7 @@
 			einfo->invert |= IPT_ECN_OP_MATCH_IP;
 		*flags |= IPT_ECN_OP_MATCH_IP;
 		einfo->operation |= IPT_ECN_OP_MATCH_IP;
-		if (string_to_number(optarg, 0, 3, &result))
+		if (string_to_number(argv[optind-1], 0, 3, &result))
 			exit_error(PARAMETER_PROBLEM,
 				   "ECN match: Value out of range");
 		einfo->ip_ect = result;

Index: extensions/libipt_helper.c
===================================================================
RCS file: /cvspublic/netfilter/userspace/extensions/libipt_helper.c,v retrieving revision 1.3

libipt_helper.c +2 -2
diff -u -r1.3 libipt_helper.c
@@ -44,8 +44,8 @@
 
 	switch (c) {
 	case '1':
-		check_inverse(optarg, &invert, &invert, 0);
-		strncpy(info->name, optarg, 29);
+		check_inverse(optarg, &invert, &optind, 0);
+		strncpy(info->name, argv[optind-1], 29);
 		if (invert)
 			info->invert = 1;
 		*flags = 1;

Index: extensions/libipt_mark.c
===================================================================
RCS file: /cvspublic/netfilter/userspace/extensions/libipt_mark.c,v retrieving revision 1.8

libipt_mark.c +3 -3
diff -u -r1.8 libipt_mark.c
@@ -46,13 +46,13 @@
 		char *end;
 	case '1':
 		check_inverse(optarg, &invert, &optind, 0);
-		markinfo->mark = strtoul(optarg, &end, 0);
+		markinfo->mark = strtoul(argv[optind-1], &end, 0);
 		if (*end == '/') {
 			markinfo->mask = strtoul(end+1, &end, 0);
 		} else
 			markinfo->mask = 0xffffffff;
-		if (*end != '\0' || end == optarg)
-			exit_error(PARAMETER_PROBLEM, "Bad MARK value `%s'", optarg);
+		if (*end != '\0' || end == argv[optind-1])
+			exit_error(PARAMETER_PROBLEM, "Bad MARK value `%s'", argv[optind-1]);
 		if (invert)
 			markinfo->invert = 1;
 		*flags = 1;

Index: extensions/libipt_owner.c
===================================================================
RCS file: /cvspublic/netfilter/userspace/extensions/libipt_owner.c,v retrieving revision 1.8

libipt_owner.c +18 -17
diff -u -r1.8 libipt_owner.c
@@ -71,12 +71,12 @@
 		struct group *grp;
 	case '1':
 		check_inverse(optarg, &invert, &optind, 0);
-		if ((pwd = getpwnam(optarg)))
+		if ((pwd = getpwnam(argv[optind-1])))
 			ownerinfo->uid = pwd->pw_uid;
 		else {
-			ownerinfo->uid = strtoul(optarg, &end, 0);
-			if (*end != '\0' || end == optarg)
-				exit_error(PARAMETER_PROBLEM, "Bad OWNER UID value `%s'", optarg);
+			ownerinfo->uid = strtoul(argv[optind-1], &end, 0);
+			if (*end != '\0' || end == argv[optind-1])
+				exit_error(PARAMETER_PROBLEM, "Bad OWNER UID value `%s'", argv[optind-1]);
 		}
 		if (invert)
 			ownerinfo->invert |= IPT_OWNER_UID;

@@ -86,12 +86,12 @@
 
 	case '2':
 		check_inverse(optarg, &invert, &optind, 0);
-		if ((grp = getgrnam(optarg)))
+		if ((grp = getgrnam(argv[optind-1])))
 			ownerinfo->gid = grp->gr_gid;
 		else {
-			ownerinfo->gid = strtoul(optarg, &end, 0);
-			if (*end != '\0' || end == optarg)
-				exit_error(PARAMETER_PROBLEM, "Bad OWNER GID value `%s'", optarg);
+			ownerinfo->gid = strtoul(argv[optind-1], &end, 0);
+			if (*end != '\0' || end == argv[optind-1])
+				exit_error(PARAMETER_PROBLEM, "Bad OWNER GID value `%s'", argv[optind-1]);
 		}
 		if (invert)
 			ownerinfo->invert |= IPT_OWNER_GID;

@@ -101,9 +101,9 @@
 
 	case '3':
 		check_inverse(optarg, &invert, &optind, 0);
-		ownerinfo->pid = strtoul(optarg, &end, 0);
-		if (*end != '\0' || end == optarg)
-			exit_error(PARAMETER_PROBLEM, "Bad OWNER PID value `%s'", optarg);
+		ownerinfo->pid = strtoul(argv[optind-1], &end, 0);
+		if (*end != '\0' || end == argv[optind-1])
+			exit_error(PARAMETER_PROBLEM, "Bad OWNER PID value `%s'", argv[optind-1]);
 		if (invert)
 			ownerinfo->invert |= IPT_OWNER_PID;
 		ownerinfo->match |= IPT_OWNER_PID;

@@ -112,9 +112,9 @@
 
 	case '4':
 		check_inverse(optarg, &invert, &optind, 0);
-		ownerinfo->sid = strtoul(optarg, &end, 0);
-		if (*end != '\0' || end == optarg)
-			exit_error(PARAMETER_PROBLEM, "Bad OWNER SID value `%s'", optarg);
+		ownerinfo->sid = strtoul(argv[optind-1], &end, 0);
+		if (*end != '\0' || end == argv[optind-1])
+			exit_error(PARAMETER_PROBLEM, "Bad OWNER SID value `%s'", argv[optind-1]);
 		if (invert)
 			ownerinfo->invert |= IPT_OWNER_SID;
 		ownerinfo->match |= IPT_OWNER_SID;

@@ -124,10 +124,11 @@
 #ifdef IPT_OWNER_COMM
 	case '5':
 		check_inverse(optarg, &invert, &optind, 0);
-		if(strlen(optarg) > sizeof(ownerinfo->comm))
-			exit_error(PARAMETER_PROBLEM, "OWNER CMD `%s' too long, max %d characters", optarg, sizeof(ownerinfo->comm));
+		if(strlen(argv[optind-1]) > sizeof(ownerinfo->comm))
+			exit_error(PARAMETER_PROBLEM, "OWNER CMD `%s' too long, max %d characters",
+			           argv[optind-1], sizeof(ownerinfo->comm));
 
-		strncpy(ownerinfo->comm, optarg, sizeof(ownerinfo->comm));
+		strncpy(ownerinfo->comm, argv[optind-1], sizeof(ownerinfo->comm));
 
 		if (invert)
 			ownerinfo->invert |= IPT_OWNER_COMM;

Index: extensions/libipt_realm.c
===================================================================
RCS file: /cvspublic/netfilter/userspace/extensions/libipt_realm.c,v retrieving revision 1.4

libipt_realm.c +3 -3
diff -u -r1.4 libipt_realm.c
@@ -50,13 +50,13 @@
 		char *end;
 	case '1':
 		check_inverse(optarg, &invert, &optind, 0);
-		realminfo->id = strtoul(optarg, &end, 0);
+		realminfo->id = strtoul(argv[optind-1], &end, 0);
 		if (*end == '/') {
 			realminfo->mask = strtoul(end+1, &end, 0);
 		} else
 			realminfo->mask = 0xffffffff;
-		if (*end != '\0' || end == optarg)
-			exit_error(PARAMETER_PROBLEM, "Bad REALM value `%s'", optarg);
+		if (*end != '\0' || end == argv[optind-1])
+			exit_error(PARAMETER_PROBLEM, "Bad REALM value `%s'", argv[optind-1]);
 		if (invert)
 			realminfo->invert = 1;
 		*flags = 1;
author
#4 lore raw

Re: [PATCH]: fix all iptables problems with '!' (hopefully)

Quoted 4 lines · Patrick McHardy

I did, here is the (untested) patch, hopefully i didn't miss any. I didn't change any of the "save"-functions, this patch just replaces all uses
of optarg after check_inverse by argv[optind-1].

I didn't test it, but keep in mind that the ! in certain cases has to be placed *before* the argument and its value.

For example:

--uid-owner ! squid

is not accepted either ("user '!' not found"). The correct is:

! --uid-owner squid

Regards

Signature · 8 lines
Costa Tsaousis

Author of
FireHOL (http://firehol.sf.net)
-------------------------------
An iptables stateful firewall generator,
that allows you to configure your firewall...
...the way you think of it!
#5 lore raw

Re: [PATCH]: fix all iptables problems with '!' (hopefully)

Quoted 13 lines · Costa Tsaousis
Quoted 6 lines · Patrick McHardy

I did, here is the (untested) patch, hopefully i didn't miss any. I didn't change any of the "save"-functions, this patch just replaces all uses
of optarg after check_inverse by argv[optind-1].

I didn't test it, but keep in mind that the ! in certain cases has to be placed *before* the argument and its value.

Quoted 9 lines

For example:

--uid-owner ! squid

is not accepted either ("user '!' not found"). The correct is:

! --uid-owner squid

In fact ! is used in many matches in a non-boolean context. After the patch is applied, both it always possible (! --uid-owner xxx / --uid-owner ! xxx).
It would still be nice to have the helptexts unified concerning the use of '!', i think.

Regards,
Patrick

#6 lore raw

Re: [PATCH]: fix all iptables problems with '!' (hopefully)

Quoted 5 lines · Patrick McHardy

In fact ! is used in many matches in a non-boolean context. After the patch is applied, both it always possible (! --uid-owner xxx / --uid-owner ! xxx).
It would still be nice to have the helptexts unified concerning the use of '!', i think.

This is why I'm still unsure whether to apply your patch or not.

I'd rather like to enforce one unique syntax than offering two different options giving the same result (and possibly causing confusion with the user).

Quoted 2 lines

Regards,
Patrick

Signature · 4 lines
- Harald Welte / [email protected]               http://www.gnumonks.org/
============================================================================
"If this were a dictatorship, it'd be a heck of a lot easier, just so long
 as I'm the dictator."  --  George W. Bush Dec 18, 2000
#7 lore raw

Re: [PATCH]: fix all iptables problems with '!' (hopefully)

Quoted 11 lines · Harald Welte
Quoted 7 lines · Patrick McHardy

In fact ! is used in many matches in a non-boolean context. After the patch is applied, both it always possible (! --uid-owner xxx / --uid-owner ! xxx).
It would still be nice to have the helptexts unified concerning the use of '!', i think.

Quoted 7 lines

This is why I'm still unsure whether to apply your patch or not.

I'd rather like to enforce one unique syntax than offering two different options giving the same result (and possibly causing confusion with the user).

Thats the reason why i was unsure weather to send the patch or not ;) What about if i convert all matches to only show "!" in boolean context in the helptexts and change check_inverse and what else might be neccessary to enforce this ? What about backward-compatibility ?

Regards,
Patrick

#8 lore raw

Re: [PATCH]: fix all iptables problems with '!' (hopefully)

Quoted 1 line · Patrick McHardy

Thats the reason why i was unsure weather to send the patch or not ;)

I see.

Quoted 3 lines

What about if i convert all matches to only show "!" in boolean context in the helptexts and change check_inverse and what else might be neccessary to enforce this ?

I think this is a wise idea.

Quoted 1 line

What about backward-compatibility ?

This is usually not my idea of being user-friendly... but considering how much trouble we've had with this over the last couple of months, I'm willing to sacrifice a bit of backwards compatibility.

To everyone: Please feel free to protest.

Quoted 2 lines

Regards,
Patrick

Signature · 4 lines
- Harald Welte / [email protected]               http://www.gnumonks.org/
============================================================================
"If this were a dictatorship, it'd be a heck of a lot easier, just so long
 as I'm the dictator."  --  George W. Bush Dec 18, 2000
#9 lore raw

Re: [PATCH]: fix all iptables problems with '!' (hopefully)

Quoted 8 lines · Harald Welte
Quoted 4 lines

What about if i convert all matches to only show "!" in boolean context in the helptexts and change check_inverse and what else might be neccessary to enforce this ?

I think this is a wise idea.

Since i have to touch a lot of helptexts anyway i thought it might be the time to unify their format. Any wishes regarding this ?

Regards,
Patrick

Quoted 14 lines
Quoted 5 lines

What about backward-compatibility ?

This is usually not my idea of being user-friendly... but considering how much trouble we've had with this over the last couple of months, I'm willing to sacrifice a bit of backwards compatibility.

To everyone: Please feel free to protest.

#10 lore raw
In reply to Hervé Eychenne
Quoted 8 lines · Hervé Eychenne

On Mon, Jan 06, 2003 at 03:31:07AM +0200, Costa Tsaousis wrote:

Hi,

Quoted 4 lines

I believe I have faced another, not-mentioned-in-2002, bug:
[...]
then iptables-save gives this:

Quoted 8 lines
Quoted 4 lines

-A OUTPUT -d ! 127.0.0.1 -p tcp -m owner --uid-owner !squid -m tcp --dport 80 -j DNAT --to-destination 127.0.0.1:3128

which produces a "user '!squid' not found" error when restored.

It seems to me that no single week passes without any post about this kind of error (no space before a negation) in some save output module. Sigh.

Your observation is true.

Quoted 3 lines

I must admit it seems quite strange to me that noone in the coreteam didn't do that already. (hope I won't hurt anybody's feelings too much though) ;-)

The problem is that we are mostly talking about contributed extensions, and nobody seems to test the save() and restore() functions of their extensions properly.

Quoted 3 lines

Would someone who's even more fed up than me with these recurrent messages have time to audit the whole bunch of existing modules in the CVS tree and fix this particular kind of problem _once and for all_?

It can even be automatized by a script...

Quoted 2 lines

Thanks in advance,
Herve

Signature · 4 lines
- Harald Welte / [email protected]               http://www.gnumonks.org/
============================================================================
"If this were a dictatorship, it'd be a heck of a lot easier, just so long
 as I'm the dictator."  --  George W. Bush Dec 18, 2000