iptables: handle cidr notation more sanely

PATCH Applied Phil Oester 3 participants 6 messages Jul 9, 2006 – Jul 10, 2006 netfilter

lore

Share & tools

lore

Phil Oester

author

2006-07-09 22:28 UTC #0 lore raw

PATCH 1/1 iptables: handle cidr notation more sanely

At present, a command such as

	iptables -A foo -s 10.10/16

will interpret 10.10/16 as 10.0.0.10/16, and after applying the mask end up with 10.0.0.0/16, which likely isn't what the user intended. Yet some people do expect 10.10 (without the cidr notation) to end up as 10.0.0.10.

The below patch should satisfy all parties. It zero pads the missing octets only in the cidr case, leaving the IP untouched otherwise.

This resolves bug #422

Phil

iptables.c +30 -0

diff -ru ipt-orig/iptables.c ipt-new/iptables.c
@@ -583,6 +583,34 @@
 	return (char *) NULL;
 }
 
+static void 
+pad_cidr(char *cidr)
+{
+	char *p, *q;
+	unsigned int onebyte;
+	int i, j;
+	char buf[20];
+
+	/* copy dotted string, because we need to modify it */
+	strncpy(buf, cidr, sizeof(buf) - 1);
+	buf[sizeof(buf) - 1] = '\0';
+
+	p = buf;
+	for (i = 0; i <= 3; i++) {
+		if ((q = strchr(p, '.')) == NULL)
+			break;
+		*q = '\0';
+		if (string_to_number(p, 0, 255, &onebyte) == -1)
+			return;
+		p = q + 1;
+	}
+
+	/* pad remaining octets with zeros */
+	for (j = i; j < 3; j++) {
+		strcat(cidr, ".0");
+	}
+}
+
 /*
  *	All functions starting with "parse" should succeed, otherwise
  *	the program fails.

@@ -651,6 +679,8 @@
 	if ((p = strrchr(buf, '/')) != NULL) {
 		*p = '\0';
 		addrp = parse_mask(p + 1);
+		if (strrchr(p + 1, '.') == NULL)
+			pad_cidr(buf);
 	} else
 		addrp = parse_mask(NULL);
 	inaddrcpy(maskp, addrp);

Patrick McHardy

Applied

2006-07-10 04:26 UTC #1 lore raw

Quoted 9 lines · Phil Oester

At present, a command such as
	iptables -A foo -s 10.10/16
will interpret 10.10/16 as 10.0.0.10/16, and after applying the mask end up with 10.0.0.0/16, which likely isn't what the user intended. Yet some people do expect 10.10 (without the cidr notation) to end up as 10.0.0.10.

Quoted 4 lines

The below patch should satisfy all parties. It zero pads the missing octets only in the cidr case, leaving the IP untouched otherwise.
This resolves bug #422

Applied, thanks Phil. Hope all those lazy typers are happy now :)

Patrick Schaaf

2006-07-10 05:59 UTC #2 lore raw

Quoted 6 lines

Quoted 6 lines
	iptables -A foo -s 10.10/16
will interpret 10.10/16 as 10.0.0.10/16, and after applying the mask end up with 10.0.0.0/16, which likely isn't what the user intended. Yet some people do expect 10.10 (without the cidr notation) to end up as 10.0.0.10.

...

Quoted 2 lines

Applied, thanks Phil. Hope all those lazy typers are happy now :)

Better hope the past lazy typers' boot time iptables scripts will not break in any critical way by such a radical interpretation change.

Of course, it's all their own fault when that happens, lazy bastards.

Methinks that it would be better to make non-3-dots IP addresses a syntax error, period.

best regards
Patrick

Patrick McHardy

2006-07-10 06:43 UTC #3 lore raw

Quoted 9 lines · Patrick Schaaf

Quoted 6 lines
Quoted 6 lines
	iptables -A foo -s 10.10/16
will interpret 10.10/16 as 10.0.0.10/16, and after applying the mask end up with 10.0.0.0/16, which likely isn't what the user intended. Yet some people do expect 10.10 (without the cidr notation) to end up as 10.0.0.10.
...

Quoted 7 lines

Quoted 1 line
Applied, thanks Phil. Hope all those lazy typers are happy now :)
Better hope the past lazy typers' boot time iptables scripts will not break in any critical way by such a radical interpretation change.
Of course, it's all their own fault when that happens, lazy bastards.

Please don't put words in my mouth. If they rely on undocumented, clearly illogical (10.10/16 == 10/16) behaviour, then yes, they are at fault, but its not because of beeing lazy typers. Anyway, if I'd believe that this is something more than a handful of people were actually using and that _anyone_ would actually rely on the old behaviour, I wouldn't have accepted that patch. So if anyone does report breakage until the next iptables release, I'll most likely revert the patch.

Quoted 2 lines

Methinks that it would be better to make non-3-dots IP addresses a syntax error, period.

I can understand your position about breaking compatibility, but this seems unreasonable to me. You're saying we should do something that has even more problems (breaking compatibility for shortcuts without masks as well), but less value to the user?

Patrick Schaaf

2006-07-10 06:52 UTC #4 lore raw

Quoted 6 lines

Quoted 4 lines
Better hope the past lazy typers' boot time iptables scripts will not break in any critical way by such a radical interpretation change.
Of course, it's all their own fault when that happens, lazy bastards.
Please don't put words in my mouth.

Those were my words, not yours. I did in no way intend to put them into your mouth. And I stand by them. I'm a cynic. That's one of the reasons I forbid myself becoming maintainer... (lazyness being the most important reason, of course :)

Quoted 3 lines

If they rely on undocumented, clearly illogical (10.10/16 == 10/16) behaviour, then yes, they are at fault, but its not because of beeing lazy typers.

On one hand, it is just that, in my opinion.

On the other hand, it is always a bit the fault of software permitting such abbreviation with arbitrary, undocumented semantics, in the first place.

Quoted 4 lines

Anyway,
if I'd believe that this is something more than a handful of people were actually using and that _anyone_ would actually rely on the old behaviour, I wouldn't have accepted that patch.

That's fine with me. You are the maintainer. There is no better judge for such things. I just couldn't stop myself from adding a piece of opinion.

Quoted 7 lines

Quoted 2 lines
Methinks that it would be better to make non-3-dots IP addresses a syntax error, period.
I can understand your position about breaking compatibility, but this seems unreasonable to me. You're saying we should do something that has even more problems (breaking compatibility for shortcuts without masks as well),

I did not think about the case without masks (/32) at all. Good point.

So methinks non-3-dots should be syntax errors whenever used with masks != /32.

And you do what you think is best. As always. And I thank you for that!

best regards
Patrick

Patrick McHardy

2006-07-10 07:24 UTC #5 lore raw

Quoted 5 lines · Patrick Schaaf

Quoted 5 lines
Quoted 4 lines
Better hope the past lazy typers' boot time iptables scripts will not break in any critical way by such a radical interpretation change.
Of course, it's all their own fault when that happens, lazy bastards.

Quoted 7 lines

Quoted 1 line
Please don't put words in my mouth.
Those were my words, not yours. I did in no way intend to put them into your mouth. And I stand by them. I'm a cynic. That's one of the reasons I forbid myself becoming maintainer... (lazyness being the most important reason, of course :)

Applogogies for the misunderstanding then.

Quoted 9 lines

Quoted 3 lines
If they rely on undocumented, clearly illogical (10.10/16 == 10/16) behaviour, then yes, they are at fault, but its not because of beeing lazy typers.
On one hand, it is just that, in my opinion.
On the other hand, it is always a bit the fault of software permitting such abbreviation with arbitrary, undocumented semantics, in the first place.

I absolutely agree, my guess is that my feelings about that are even stronger than yours :) This is one reason why I think it is good to get rid of the old behaviour.

Quoted 9 lines

Quoted 7 lines
Quoted 2 lines
Methinks that it would be better to make non-3-dots IP addresses a syntax error, period.
I can understand your position about breaking compatibility, but this seems unreasonable to me. You're saying we should do something that has even more problems (breaking compatibility for shortcuts without masks as well),

Quoted 3 lines

I did not think about the case without masks (/32) at all. Good point.
So methinks non-3-dots should be syntax errors whenever used with masks != /32.

I actually think its nice that we now support 172.16/16 in a way I think a user would expect it to work. The important thing is that is has well defined and commonly understood semantics (and maybe that it gets documented). The old behaviour, which is now only used for non-masked expressions was unknown to me (10.10 == 10.0.0.10), but it matches what IPv6 does and according to Henrik (whom I fully trust) it is also commonly used for IPv4. So I think what we have with this patch fully meets user expectations.

Quoted 1 line

And you do what you think is best. As always. And I thank you for that!

Unfortunately I also don't always know what's best, so your opinion is appreciated, forcing me to think for a second time can only be a good thing :)