[iptables PATCH 1/3] extensions: libipt_icmp: Fix confusion between 255/255 and any

[iptables PATCH 1/3] extensions: libipt_icmp: Fix confusion between 255/255 and any

[Phil Sutter]

Per definition, ICMP type "any" is type 255 and the full range of codes
(0-255). Save callback though ignored the actual code values, printing
"any" for every type 255 match. This at least confuses users as they
can't find their rule added as '--icmp-type 255/255' anymore.

It is not entirely clear what the fixed commit was trying to establish,
but the save output is certainly not correct (especially since print
callback gets things right).

Reported-by: Amelia Downs <adowns@vmware.com>
Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1600
Fixes: fc9237da4e845 ("Fix '-p icmp -m icmp' issue (Closes: #37)")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 extensions/libipt_icmp.c | 3 ++-
 extensions/libipt_icmp.t | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/extensions/libipt_icmp.c b/extensions/libipt_icmp.c
index b0318aebc2c57..171b3b3949e54 100644
--- a/extensions/libipt_icmp.c
+++ b/extensions/libipt_icmp.c
@@ -108,7 +108,8 @@ static void icmp_save(const void *ip, const struct xt_entry_match *match)
 		printf(" !");
 
 	/* special hack for 'any' case */
-	if (icmp->type == 0xFF) {
+	if (icmp->type == 0xFF &&
+	    icmp->code[0] == 0 && icmp->code[1] == 0xFF) {
 		printf(" --icmp-type any");
 	} else {
 		printf(" --icmp-type %u", icmp->type);
diff --git a/extensions/libipt_icmp.t b/extensions/libipt_icmp.t
index f4ba65c27f032..ce4a33f9633b5 100644
--- a/extensions/libipt_icmp.t
+++ b/extensions/libipt_icmp.t
@@ -13,3 +13,5 @@
 # we accept "iptables -I INPUT -p tcp -m tcp", why not this below?
 # ERROR: cannot load: iptables -A INPUT -p icmp -m icmp
 # -p icmp -m icmp;=;OK
+-p icmp -m icmp --icmp-type 255/255;=;OK
+-p icmp -m icmp --icmp-type 255/0:255;-p icmp -m icmp --icmp-type any;OK
-- 
2.40.0

[iptables PATCH 2/3] extensions: libipt_icmp: --icmp-type is not mandatory

[Phil Sutter]

The init callback sets things up for us, not specifying --icmp-type
results in an '--icmp-type any' match which seems perfectly fine.

Fixes: 1b8db4f4ca250 ("libip[6]t_icmp: use guided option parser")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 extensions/libipt_icmp.c | 2 +-
 extensions/libipt_icmp.t | 4 +---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/extensions/libipt_icmp.c b/extensions/libipt_icmp.c
index 171b3b3949e54..29c9e1a6cd727 100644
--- a/extensions/libipt_icmp.c
+++ b/extensions/libipt_icmp.c
@@ -31,7 +31,7 @@ static void icmp_help(void)
 
 static const struct xt_option_entry icmp_opts[] = {
 	{.name = "icmp-type", .id = O_ICMP_TYPE, .type = XTTYPE_STRING,
-	 .flags = XTOPT_MAND | XTOPT_INVERT},
+	 .flags = XTOPT_INVERT},
 	XTOPT_TABLEEND,
 };
 
diff --git a/extensions/libipt_icmp.t b/extensions/libipt_icmp.t
index ce4a33f9633b5..08692900dba12 100644
--- a/extensions/libipt_icmp.t
+++ b/extensions/libipt_icmp.t
@@ -10,8 +10,6 @@
 # ERROR: cannot load: iptables -A INPUT -p icmp -m icmp --icmp-type destination-unreachable/network-unreachable
 # -p icmp -m icmp --icmp-type destination-unreachable/network-unreachable;=;OK
 -m icmp;;FAIL
-# we accept "iptables -I INPUT -p tcp -m tcp", why not this below?
-# ERROR: cannot load: iptables -A INPUT -p icmp -m icmp
-# -p icmp -m icmp;=;OK
+-p icmp -m icmp;-p icmp -m icmp --icmp-type any;OK
 -p icmp -m icmp --icmp-type 255/255;=;OK
 -p icmp -m icmp --icmp-type 255/0:255;-p icmp -m icmp --icmp-type any;OK
-- 
2.40.0

[iptables PATCH 3/3] tests: libipt_icmp.t: Enable tests with numeric output

[Phil Sutter]

Unrelated to the question whether numeric (save) output is desired or
not, enable the tests and expect the known format.

Using --list without --numeric prints the names, BTW.

Fixes: 49d5b7277c7f2 ("extensions: libipt_icmp: add unit test")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 extensions/libipt_icmp.t | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/extensions/libipt_icmp.t b/extensions/libipt_icmp.t
index 08692900dba12..3c97cf4e06128 100644
--- a/extensions/libipt_icmp.t
+++ b/extensions/libipt_icmp.t
@@ -1,11 +1,8 @@
 :INPUT,FORWARD,OUTPUT
 -p icmp -m icmp --icmp-type any;=;OK
-# output uses the number, better use the name?
-# ERROR: cannot find: iptables -I INPUT -p icmp -m icmp --icmp-type echo-reply
-# -p icmp -m icmp --icmp-type echo-reply;=;OK
-# output uses the number, better use the name?
-# ERROR: annot find: iptables -I INPUT -p icmp -m icmp --icmp-type destination-unreachable
-# -p icmp -m icmp --icmp-type destination-unreachable;=;OK
+# XXX: output uses the number, better use the name?
+-p icmp -m icmp --icmp-type echo-reply;-p icmp -m icmp --icmp-type 0;OK
+-p icmp -m icmp --icmp-type destination-unreachable;-p icmp -m icmp --icmp-type 3;OK
 # it does not acccept name/name, should we accept this?
 # ERROR: cannot load: iptables -A INPUT -p icmp -m icmp --icmp-type destination-unreachable/network-unreachable
 # -p icmp -m icmp --icmp-type destination-unreachable/network-unreachable;=;OK
-- 
2.40.0

Re: [iptables PATCH 1/3] extensions: libipt_icmp: Fix confusion between 255/255 and any

[Jan Engelhardt]

On Wednesday 2023-08-02 04:05, Phil Sutter wrote:
>
>It is not entirely clear what the fixed commit was trying to establish,
>but the save output is certainly not correct (especially since print
>callback gets things right).
>
>Fixes: fc9237da4e845 ("Fix '-p icmp -m icmp' issue (Closes: #37)")

"""(libipt_icmp.c):
 * Up to kernel <=2.4.20 the problem was:
 * '-p icmp ' matches all icmp packets
 * '-p icmp -m icmp' matches _only_ ICMP type 0 :(
 * This is now fixed by initializing the field * to icmp type 0xFF
"""

But also:

v1.2.7a-35-gfc9237da missed touching *libip6t_icmp6.c*, so
it was never updated with the same "bug".

In icmp6, --icmpv6-type was (and still is to this date) mandatory, which means
`-p icmp6 -m icmp6` would *not* implicitly go match ICMP(v6) type 0 like its
IPv4 counterpart.

Then, in v1.4.10-115-g1b8db4f4, libipt_icmp.c more or less accidentally gained
XTOPT_MAND (possible spill from IPv6 code), therefore `-p icmp -m icmp` would
also stop implicitly doing ICMP type "any".

Re: [iptables PATCH 1/3] extensions: libipt_icmp: Fix confusion between 255/255 and any

[Phil Sutter]

On Wed, Aug 02, 2023 at 10:31:16AM +0200, Jan Engelhardt wrote:
> On Wednesday 2023-08-02 04:05, Phil Sutter wrote:
> >
> >It is not entirely clear what the fixed commit was trying to establish,
> >but the save output is certainly not correct (especially since print
> >callback gets things right).
> >
> >Fixes: fc9237da4e845 ("Fix '-p icmp -m icmp' issue (Closes: #37)")
> 
> """(libipt_icmp.c):
>  * Up to kernel <=2.4.20 the problem was:
>  * '-p icmp ' matches all icmp packets
>  * '-p icmp -m icmp' matches _only_ ICMP type 0 :(
>  * This is now fixed by initializing the field * to icmp type 0xFF
> """
> 
> But also:
> 
> v1.2.7a-35-gfc9237da missed touching *libip6t_icmp6.c*, so
> it was never updated with the same "bug".
> 
> In icmp6, --icmpv6-type was (and still is to this date) mandatory, which means
> `-p icmp6 -m icmp6` would *not* implicitly go match ICMP(v6) type 0 like its
> IPv4 counterpart.
> 
> Then, in v1.4.10-115-g1b8db4f4, libipt_icmp.c more or less accidentally gained
> XTOPT_MAND (possible spill from IPv6 code), therefore `-p icmp -m icmp` would
> also stop implicitly doing ICMP type "any".

Thanks for the forensics.

One could extend icmp6 match (in kernel and user space) to support this
"any" type, though it seems not guaranteed the value 255 won't be used
for a real message at some point. So a proper solution was to support type
ranges like ebtables does. Then "any" type is 0:255/0:255.

Apart from the above, the three patches of this series should be fine,
right?

Thanks, Phil

Re: [iptables PATCH 1/3] extensions: libipt_icmp: Fix confusion between 255/255 and any

[Jan Engelhardt]

On Thursday 2023-08-03 14:57, Phil Sutter wrote:
>> >
>> >It is not entirely clear what the fixed commit was trying to establish,
>> >but the save output is certainly not correct (especially since print
>> >callback gets things right).
>> 
>> v1.2.7a-35-gfc9237da missed touching *libip6t_icmp6.c*, so
>> it was never updated with the same "bug".[...]
>
>One could extend icmp6 match (in kernel and user space) to support this
>"any" type, though it seems not guaranteed the value 255 won't be used
>for a real message at some point. So a proper solution was to support type
>ranges like ebtables does. Then "any" type is 0:255/0:255.
>
>Apart from the above, the three patches of this series should be fine,
>right?

Yeah.

Re: [iptables PATCH 1/3] extensions: libipt_icmp: Fix confusion between 255/255 and any

[Phil Sutter]

On Thu, Aug 03, 2023 at 09:38:47PM +0200, Jan Engelhardt wrote:
> 
> On Thursday 2023-08-03 14:57, Phil Sutter wrote:
> >> >
> >> >It is not entirely clear what the fixed commit was trying to establish,
> >> >but the save output is certainly not correct (especially since print
> >> >callback gets things right).
> >> 
> >> v1.2.7a-35-gfc9237da missed touching *libip6t_icmp6.c*, so
> >> it was never updated with the same "bug".[...]
> >
> >One could extend icmp6 match (in kernel and user space) to support this
> >"any" type, though it seems not guaranteed the value 255 won't be used
> >for a real message at some point. So a proper solution was to support type
> >ranges like ebtables does. Then "any" type is 0:255/0:255.
> >
> >Apart from the above, the three patches of this series should be fine,
> >right?
> 
> Yeah.

I have applied the series apart from patch 2: The wrong XTOPT_MAND flag
is present since v1.4.11, if anyone actually depends on the behaviour,
they would have complained. So leaving things as they are is fine, and
instead one should go into another direction:

- Drop XTOPT_MAND and instead provide a better error message in
  x6_fcheck callback, pointing out that '-p icmp -m icmp' is equivalent
  to plain '-p icmp'.

- Same for '--icmp-type any'? In theory, it's pointless.

A nicer option would be to turn '-m icmp [--icmp-type any]' into a NOP.
Using '-p icmp' is mandatory anyway. There's no support for matches to
remove themselves, though. I guess this could be done by extending
xtables_option_mfcall().

Cheers, Phil