[PATCH] icmpv6 match --icmpv6-type confusion

[PATCH] icmpv6 match --icmpv6-type confusion

[Phil Oester]

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

Adding a rule such as this:

-A INPUT -p ipv6-icmp -m icmpv6

ends up creating a rule with "ipv6-icmp type 0", instead of "ipv6-icmp type any"
which is what was expected.  Granted, the match is redundant anyway, but it
should not assume a specific type when one is not specified.

Below patches (to both kernel and userspace) fix this, and resolve bug #461.
In keeping with icmpv4, 0xFF is used to designate "any". And since I was
looking at it, I fixed up a 255 -> any in icmpv4 printing.

Phil

Signed-off-by: Phil Oester <kernel@linuxace.com>



[-- Attachment #2: patch-icmpv6-kernel --]
[-- Type: text/plain, Size: 427 bytes --]

--- linux-std/net/ipv6/netfilter/ip6_tables.c	2006-06-17 21:49:35.000000000 -0400
+++ linux-po/net/ipv6/netfilter/ip6_tables.c	2006-06-29 23:05:09.000000000 -0400
@@ -1308,7 +1308,7 @@
 		     u_int8_t type, u_int8_t code,
 		     int invert)
 {
-	return (type == test_type && code >= min_code && code <= max_code)
+	return ((test_type == 0xFF) || (type == test_type && code >= min_code && code <= max_code))
 		^ invert;
 }
 

[-- Attachment #3: patch-icmpv6-user --]
[-- Type: text/plain, Size: 1557 bytes --]

diff -ru ipt-orig/extensions/libip6t_icmp6.c ipt-new/extensions/libip6t_icmp6.c
--- ipt-orig/extensions/libip6t_icmp6.c	2006-04-14 20:05:41.000000000 -0700
+++ ipt-new/extensions/libip6t_icmp6.c	2006-06-29 18:07:19.000000000 -0700
@@ -149,6 +149,7 @@
 {
 	struct ip6t_icmp *icmpv6info = (struct ip6t_icmp *)m->data;
 
+	icmpv6info->type = 0xFF;
 	icmpv6info->code[1] = 0xFF;
 }
 
@@ -206,7 +207,12 @@
 	if (invert)
 		printf("!");
 
-	printf("type %u", type);
+	/* special hack for 'any' case */
+	if (type == 0xFF)
+		printf("type any ");
+	else
+		printf("type %u", type);
+
 	if (code_min == 0 && code_max == 0xFF)
 		printf(" ");
 	else if (code_min == code_max)
@@ -241,7 +247,12 @@
 	if (icmpv6->invflags & IP6T_ICMP_INV)
 		printf("! ");
 
-	printf("--icmpv6-type %u", icmpv6->type);
+	/* special hack for 'any' case */
+	if (icmpv6->type == 0xFF)
+		printf("--icmpv6-type any ");
+	else
+		printf("--icmpv6-type %u", icmpv6->type);
+
 	if (icmpv6->code[0] != 0 || icmpv6->code[1] != 0xFF)
 		printf("/%u", icmpv6->code[0]);
 	printf(" ");
diff -ru ipt-orig/extensions/libipt_icmp.c ipt-new/extensions/libipt_icmp.c
--- ipt-orig/extensions/libipt_icmp.c	2005-02-14 05:13:04.000000000 -0800
+++ ipt-new/extensions/libipt_icmp.c	2006-06-29 17:57:47.000000000 -0700
@@ -231,7 +231,12 @@
 	if (invert)
 		printf("!");
 
-	printf("type %u", type);
+	/* special hack for 'any' case */
+	if (type == 0xFF)
+		printf("type any ");
+	else
+		printf("type %u", type);
+
 	if (code_min == 0 && code_max == 0xFF)
 		printf(" ");
 	else if (code_min == code_max)

Re: [PATCH] icmpv6 match --icmpv6-type confusion

[Patrick McHardy]

Phil Oester wrote:
> Adding a rule such as this:
> 
> -A INPUT -p ipv6-icmp -m icmpv6
> 
> ends up creating a rule with "ipv6-icmp type 0", instead of "ipv6-icmp type any"
> which is what was expected.  Granted, the match is redundant anyway, but it
> should not assume a specific type when one is not specified.
> 
> Below patches (to both kernel and userspace) fix this, and resolve bug #461.
> In keeping with icmpv4, 0xFF is used to designate "any". And since I was
> looking at it, I fixed up a 255 -> any in icmpv4 printing.

Thanks Phil. I have to admit that I never liked the ugly 255 hack,
its a perfectly valid value for the type field to use, even though
its currently not defined for IPv4 or IPv6. For IPv6 I even have
some concerns that it will stay unused for all times, the stategy
for choosing new values seems to be different from IPv4. I guess
an alternative would be to introduce a new revision.

Yasuyuki, do you have an opinion on this?

Re: [PATCH] icmpv6 match --icmpv6-type confusion

[Yasuyuki KOZAKAI]

From: Patrick McHardy <kaber@trash.net>
Subject: Re: [PATCH] icmpv6 match --icmpv6-type confusion
Date: Mon, 03 Jul 2006 19:52:23 +0200

> Phil Oester wrote:
> > Adding a rule such as this:
> > 
> > -A INPUT -p ipv6-icmp -m icmpv6
> > 
> > ends up creating a rule with "ipv6-icmp type 0", instead of "ipv6-icmp type any"
> > which is what was expected.  Granted, the match is redundant anyway, but it
> > should not assume a specific type when one is not specified.
> > 
> > Below patches (to both kernel and userspace) fix this, and resolve bug #461.
> > In keeping with icmpv4, 0xFF is used to designate "any". And since I was
> > looking at it, I fixed up a 255 -> any in icmpv4 printing.
> 
> Thanks Phil. I have to admit that I never liked the ugly 255 hack,
> its a perfectly valid value for the type field to use, even though
> its currently not defined for IPv4 or IPv6. For IPv6 I even have
> some concerns that it will stay unused for all times, the stategy
> for choosing new values seems to be different from IPv4. I guess
> an alternative would be to introduce a new revision.
> 
> Yasuyuki, do you have an opinion on this?

It sounds good to me to introduce a new revision and a new value to support
"icmpv6-type any". But can we really break behavior compatibility of
"-p ipv6-icmp -m icmpv6" ?

-- Yasuyuki Kozakai

Re: [PATCH] icmpv6 match --icmpv6-type confusion

[Patrick McHardy]

Yasuyuki KOZAKAI wrote:
> From: Patrick McHardy <kaber@trash.net>
> Subject: Re: [PATCH] icmpv6 match --icmpv6-type confusion
> Date: Mon, 03 Jul 2006 19:52:23 +0200
>>Phil Oester wrote:
>>
>>>Adding a rule such as this:
>>>
>>>-A INPUT -p ipv6-icmp -m icmpv6
>>>
>>>ends up creating a rule with "ipv6-icmp type 0", instead of "ipv6-icmp type any"
>>>which is what was expected.  Granted, the match is redundant anyway, but it
>>>should not assume a specific type when one is not specified.
>>>
>>>Below patches (to both kernel and userspace) fix this, and resolve bug #461.
>>>In keeping with icmpv4, 0xFF is used to designate "any". And since I was
>>>looking at it, I fixed up a 255 -> any in icmpv4 printing.
>>
>>Thanks Phil. I have to admit that I never liked the ugly 255 hack,
>>its a perfectly valid value for the type field to use, even though
>>its currently not defined for IPv4 or IPv6. For IPv6 I even have
>>some concerns that it will stay unused for all times, the stategy
>>for choosing new values seems to be different from IPv4. I guess
>>an alternative would be to introduce a new revision.
>>
>>Yasuyuki, do you have an opinion on this?
> 
> 
> It sounds good to me to introduce a new revision and a new value to support
> "icmpv6-type any". But can we really break behavior compatibility of
> "-p ipv6-icmp -m icmpv6" ?


I tend to agree with Phil's interpretation that this is actually
unexpected behaviour, so I don't think anyone is doing this
intentional. I would expect either a parser error or type any.

Re: [PATCH] icmpv6 match --icmpv6-type confusion

[Yasuyuki KOZAKAI]

[-- Attachment #1: Type: Text/Plain, Size: 1954 bytes --]

From: Patrick McHardy <kaber@trash.net>
Date: Tue, 04 Jul 2006 02:46:41 +0200

> Yasuyuki KOZAKAI wrote:
> > From: Patrick McHardy <kaber@trash.net>
> > Subject: Re: [PATCH] icmpv6 match --icmpv6-type confusion
> > Date: Mon, 03 Jul 2006 19:52:23 +0200
> >>Phil Oester wrote:
> >>
> >>>Adding a rule such as this:
> >>>
> >>>-A INPUT -p ipv6-icmp -m icmpv6
> >>>
> >>>ends up creating a rule with "ipv6-icmp type 0", instead of "ipv6-icmp type any"
> >>>which is what was expected.  Granted, the match is redundant anyway, but it
> >>>should not assume a specific type when one is not specified.
> >>>
> >>>Below patches (to both kernel and userspace) fix this, and resolve bug #461.
> >>>In keeping with icmpv4, 0xFF is used to designate "any". And since I was
> >>>looking at it, I fixed up a 255 -> any in icmpv4 printing.
> >>
> >>Thanks Phil. I have to admit that I never liked the ugly 255 hack,
> >>its a perfectly valid value for the type field to use, even though
> >>its currently not defined for IPv4 or IPv6. For IPv6 I even have
> >>some concerns that it will stay unused for all times, the stategy
> >>for choosing new values seems to be different from IPv4. I guess
> >>an alternative would be to introduce a new revision.
> >>
> >>Yasuyuki, do you have an opinion on this?
> > 
> > 
> > It sounds good to me to introduce a new revision and a new value to support
> > "icmpv6-type any". But can we really break behavior compatibility of
> > "-p ipv6-icmp -m icmpv6" ?
> 
> 
> I tend to agree with Phil's interpretation that this is actually
> unexpected behaviour, so I don't think anyone is doing this
> intentional. I would expect either a parser error or type any.

parser error is good idea. It's not neccesary for icmpv6 match to support
"--icmpv6-type any" because people can get same behavior with only
"-p ipv6-icmp".

How about the attached patch ? That also fixes allowing double
--icmp-type/--icmpv6-type.

-- Yasuyuki Kozakai


[-- Attachment #2: icmp.patch --]
[-- Type: Text/Plain, Size: 1472 bytes --]

diff --git a/extensions/libip6t_icmp6.c b/extensions/libip6t_icmp6.c
index a29bb38..6940d0e 100644
--- a/extensions/libip6t_icmp6.c
+++ b/extensions/libip6t_icmp6.c
@@ -164,11 +164,15 @@ parse(int c, char **argv, int invert, un
 
 	switch (c) {
 	case '1':
+		if (*flags == 1)
+			exit_error(PARAMETER_PROBLEM,
+				   "icmpv6 match: only use --icmpv6-type once!");
 		check_inverse(optarg, &invert, &optind, 0);
 		parse_icmpv6(argv[optind-1], &icmpv6info->type, 
 			     icmpv6info->code);
 		if (invert)
 			icmpv6info->invflags |= IP6T_ICMP_INV;
+		*flags = 1;
 		break;
 
 	default:
@@ -247,9 +251,11 @@ static void save(const struct ip6t_ip6 *
 	printf(" ");
 }
 
-/* Final check; we don't care. */
 static void final_check(unsigned int flags)
 {
+	if (!flags)
+		exit_error(PARAMETER_PROBLEM,
+			   "icmpv6 match: You must specify `--icmpv6-type'");
 }
 
 static struct ip6tables_match icmpv6 = {
diff --git a/extensions/libipt_icmp.c b/extensions/libipt_icmp.c
index 9d45c8c..8f22d05 100644
--- a/extensions/libipt_icmp.c
+++ b/extensions/libipt_icmp.c
@@ -189,11 +189,15 @@ parse(int c, char **argv, int invert, un
 
 	switch (c) {
 	case '1':
+		if (*flags == 1)
+			exit_error(PARAMETER_PROBLEM,
+				   "icmp match: only use --icmp-type once!");
 		check_inverse(optarg, &invert, &optind, 0);
 		parse_icmp(argv[optind-1], &icmpinfo->type, 
 			   icmpinfo->code);
 		if (invert)
 			icmpinfo->invflags |= IPT_ICMP_INV;
+		*flags = 1;
 		break;
 
 	default:

Re: [PATCH] icmpv6 match --icmpv6-type confusion

[Patrick McHardy]

Yasuyuki KOZAKAI wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Tue, 04 Jul 2006 02:46:41 +0200
> 
>>I tend to agree with Phil's interpretation that this is actually
>>unexpected behaviour, so I don't think anyone is doing this
>>intentional. I would expect either a parser error or type any.
> 
> 
> parser error is good idea. It's not neccesary for icmpv6 match to support
> "--icmpv6-type any" because people can get same behavior with only
> "-p ipv6-icmp".

Agreed. Too bad we already have this hack in the kernel for IPv4.

> How about the attached patch ? That also fixes allowing double
> --icmp-type/--icmpv6-type.

Looks good to me.