[PATCH] iptables: handle cidr notation more sanely

[PATCH] iptables: handle cidr notation more sanely

[Phil Oester]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

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



[-- Attachment #2: patch-cidr --]
[-- Type: text/plain, Size: 1058 bytes --]

diff -ru ipt-orig/iptables.c ipt-new/iptables.c
--- ipt-orig/iptables.c	2006-04-21 05:04:51.000000000 -0700
+++ ipt-new/iptables.c	2006-07-09 15:15:15.000000000 -0700
@@ -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);

Re: [PATCH] iptables: handle cidr notation more sanely

[Patrick McHardy]

Phil Oester wrote:
> 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

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

Re: [PATCH] iptables: handle cidr notation more sanely

[Patrick Schaaf]

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

Re: [PATCH] iptables: handle cidr notation more sanely

[Patrick McHardy]

Patrick Schaaf wrote:
>>>	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.
> 
> ...
> 
>>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.

> 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?

Re: [PATCH] iptables: handle cidr notation more sanely

[Patrick Schaaf]

> > 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 :)

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

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

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

Re: [PATCH] iptables: handle cidr notation more sanely

[Patrick McHardy]

Patrick Schaaf wrote:
>>>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 :)


Applogogies for the misunderstanding then.

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

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


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.

> 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 :)