src: allow update of net base w. meta l4proto icmpv6

PATCH In review Florian Westphal 2 participants 11 messages Mar 21, 2017 – Mar 29, 2017 netfilter

Share & tools

Florian Westphal

author maintainer

2017-03-21 18:54 UTC #0 lore raw

PATCH 1/1 src: allow update of net base w. meta l4proto icmpv6

nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert <cmdline>:1:50-60: Error: conflicting protocols specified: unknown vs. icmpv6

add icmpv6 to nexthdr list so base gets updated correctly.

Reported-by Thomas Woerner <[email protected]>

Signed-off-by Florian Westphal <[email protected]>

---
NB: This is STILL not correct.
nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert gets listed as
icmpv6 type nd-router-advert

because post processing removes the l3 dependency.

However, "icmpv6 type nd-router-advert" uses dependency ip6 nexthdr icmpv6
which isn't the same as meta l4proto icmpv6.

I suspect nft should always generate implicit l4 dependencies via meta in the ipv6 case, what do others think (and not autoremove 'nexthdr' check)?

src/proto.c +1 -0

index 79e9dbf2b33e..fcdfbe73c735 100644
@@ -779,6 +779,7 @@ const struct proto_desc proto_inet_service = {
 		PROTO_LINK(IPPROTO_TCP,		&proto_tcp),
 		PROTO_LINK(IPPROTO_DCCP,	&proto_dccp),
 		PROTO_LINK(IPPROTO_SCTP,	&proto_sctp),
+		PROTO_LINK(IPPROTO_ICMPV6,	&proto_icmp6),
 	},
 	.templates	= {
 		[0]	= PROTO_META_TEMPLATE("l4proto", &inet_protocol_type, NFT_META_L4PROTO, 8),

Signature · 1 line

2.10.2

Pablo Neira Ayuso

maintainer

2017-03-22 13:09 UTC #1 lore raw

Quoted 15 lines · Florian Westphal

nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert <cmdline>:1:50-60: Error: conflicting protocols specified: unknown vs. icmpv6
add icmpv6 to nexthdr list so base gets updated correctly.
Reported-by Thomas Woerner <[email protected]>
Signed-off-by Florian Westphal <[email protected]>
---
NB: This is STILL not correct.
nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert gets listed as
icmpv6 type nd-router-advert
because post processing removes the l3 dependency.

Quoted 7 lines

However, "icmpv6 type nd-router-advert" uses dependency ip6 nexthdr icmpv6
which isn't the same as meta l4proto icmpv6.
I suspect nft should always generate implicit l4 dependencies via meta in the ipv6 case, what do others think (and not autoremove 'nexthdr' check)?

I think we should use meta l4proto, ip6 nexthdr may point to some of the extension headers in the packet actually.

src/proto.c const struct proto_desc proto_inet_service = { · line 782

Quoted 9 lines

src/proto.c +1 -0

index 79e9dbf2b33e..fcdfbe73c735 100644
@@ -779,6 +779,7 @@ const struct proto_desc proto_inet_service = {
 		PROTO_LINK(IPPROTO_TCP,		&proto_tcp),
 		PROTO_LINK(IPPROTO_DCCP,	&proto_dccp),
 		PROTO_LINK(IPPROTO_SCTP,	&proto_sctp),
+		PROTO_LINK(IPPROTO_ICMPV6,	&proto_icmp6),

This also allows icmp6 from IPv4, right? I remember I mentioned this in a patch that I attached to bugzilla at some point so I didn't apply this.

Florian Westphal

author maintainer

2017-03-22 13:44 UTC #2 lore raw

Quoted 20 lines · Pablo Neira Ayuso

Quoted 19 lines · Florian Westphal
nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert <cmdline>:1:50-60: Error: conflicting protocols specified: unknown vs. icmpv6
add icmpv6 to nexthdr list so base gets updated correctly.
Reported-by Thomas Woerner <[email protected]>
Signed-off-by Florian Westphal <[email protected]>
---
NB: This is STILL not correct.
nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert gets listed as
icmpv6 type nd-router-advert
because post processing removes the l3 dependency.
However, "icmpv6 type nd-router-advert" uses dependency ip6 nexthdr icmpv6
which isn't the same as meta l4proto icmpv6.

Quoted 6 lines

Quoted 3 lines
I suspect nft should always generate implicit l4 dependencies via meta in the ipv6 case, what do others think (and not autoremove 'nexthdr' check)?
I think we should use meta l4proto, ip6 nexthdr may point to some of the extension headers in the packet actually.

Yes.
Alright, I'll work on this change towards l4 meta.

Quoted 5 lines

src/proto.c +0 -0

index 79e9dbf2b33e..fcdfbe73c735 100644
@@ -779,6 +779,7 @@ const struct proto_desc proto_inet_service = {

Quoted 8 lines

src/proto.c const struct proto_desc proto_inet_service = { · line 782
Quoted 4 lines
src/proto.c +1 -0
 		PROTO_LINK(IPPROTO_TCP,		&proto_tcp),
 		PROTO_LINK(IPPROTO_DCCP,	&proto_dccp),
 		PROTO_LINK(IPPROTO_SCTP,	&proto_sctp),
+		PROTO_LINK(IPPROTO_ICMPV6,	&proto_icmp6),
This also allows icmp6 from IPv4, right? I remember I mentioned this in a patch that I attached to bugzilla at some point so I didn't apply this.

Yes, whats the concern with that?

Pablo Neira Ayuso

maintainer

2017-03-22 15:38 UTC #3 lore raw

Quoted 45 lines · Florian Westphal

Quoted 20 lines · Pablo Neira Ayuso
Quoted 19 lines · Florian Westphal
nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert <cmdline>:1:50-60: Error: conflicting protocols specified: unknown vs. icmpv6
add icmpv6 to nexthdr list so base gets updated correctly.
Reported-by Thomas Woerner <[email protected]>
Signed-off-by Florian Westphal <[email protected]>
---
NB: This is STILL not correct.
nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert gets listed as
icmpv6 type nd-router-advert
because post processing removes the l3 dependency.
However, "icmpv6 type nd-router-advert" uses dependency ip6 nexthdr icmpv6
which isn't the same as meta l4proto icmpv6.
Quoted 6 lines
Quoted 3 lines
I suspect nft should always generate implicit l4 dependencies via meta in the ipv6 case, what do others think (and not autoremove 'nexthdr' check)?
I think we should use meta l4proto, ip6 nexthdr may point to some of the extension headers in the packet actually.
Yes.
Alright, I'll work on this change towards l4 meta.
Quoted 5 lines
Quoted 5 lines
src/proto.c +0 -0
index 79e9dbf2b33e..fcdfbe73c735 100644
@@ -779,6 +779,7 @@ const struct proto_desc proto_inet_service = {
Quoted 8 lines
src/proto.c const struct proto_desc proto_inet_service = { · line 782
Quoted 4 lines
src/proto.c +1 -0
 		PROTO_LINK(IPPROTO_TCP,		&proto_tcp),
 		PROTO_LINK(IPPROTO_DCCP,	&proto_dccp),
 		PROTO_LINK(IPPROTO_SCTP,	&proto_sctp),
+		PROTO_LINK(IPPROTO_ICMPV6,	&proto_icmp6),
This also allows icmp6 from IPv4, right? I remember I mentioned this in a patch that I attached to bugzilla at some point so I didn't apply this.

Quoted 1 line

Yes, whats the concern with that?

Not a problem these days from inet/netdev chains since:

commit 0011985554e269e1cc8f8e5b41eb9dcd795ebe8c

Author: Arturo Borrero Gonzalez <[email protected]>
Date:   Wed Jan 25 12:51:08 2017 +0100

payload: explicit network ctx assignment for icmp/icmp6 in special families

Now we generate the right bytecode to restrict this to IPv6:

# nft --debug=netlink add rule inet f i icmpv6 type nd-router-advert
inet f i 
  [ meta load nfproto => reg 1 ]
  [ cmp eq reg 1 0x0000000a ]
  [ payload load 1b @ network header + 6 => reg 1 ]
  [ cmp eq reg 1 0x0000003a ]
  [ payload load 1b @ transport header + 0 => reg 1 ]
  [ cmp eq reg 1 0x00000086 ]

So forget my concern, just remove this mental "postit" note, it's stale ;)

Pablo Neira Ayuso

maintainer

2017-03-22 15:38 UTC #4 lore raw

Quoted 67 lines · Pablo Neira Ayuso

Quoted 45 lines · Florian Westphal
Quoted 20 lines · Pablo Neira Ayuso
Quoted 19 lines · Florian Westphal
nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert <cmdline>:1:50-60: Error: conflicting protocols specified: unknown vs. icmpv6
add icmpv6 to nexthdr list so base gets updated correctly.
Reported-by Thomas Woerner <[email protected]>
Signed-off-by Florian Westphal <[email protected]>
---
NB: This is STILL not correct.
nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert gets listed as
icmpv6 type nd-router-advert
because post processing removes the l3 dependency.
However, "icmpv6 type nd-router-advert" uses dependency ip6 nexthdr icmpv6
which isn't the same as meta l4proto icmpv6.
Quoted 6 lines
Quoted 3 lines
I suspect nft should always generate implicit l4 dependencies via meta in the ipv6 case, what do others think (and not autoremove 'nexthdr' check)?
I think we should use meta l4proto, ip6 nexthdr may point to some of the extension headers in the packet actually.
Yes.
Alright, I'll work on this change towards l4 meta.
Quoted 5 lines
Quoted 5 lines
src/proto.c +0 -0
index 79e9dbf2b33e..fcdfbe73c735 100644
@@ -779,6 +779,7 @@ const struct proto_desc proto_inet_service = {
Quoted 8 lines
src/proto.c const struct proto_desc proto_inet_service = { · line 782
Quoted 4 lines
src/proto.c +1 -0
 		PROTO_LINK(IPPROTO_TCP,		&proto_tcp),
 		PROTO_LINK(IPPROTO_DCCP,	&proto_dccp),
 		PROTO_LINK(IPPROTO_SCTP,	&proto_sctp),
+		PROTO_LINK(IPPROTO_ICMPV6,	&proto_icmp6),
This also allows icmp6 from IPv4, right? I remember I mentioned this in a patch that I attached to bugzilla at some point so I didn't apply this.
Quoted 1 line
Yes, whats the concern with that?
Not a problem these days from inet/netdev chains since:
commit 0011985554e269e1cc8f8e5b41eb9dcd795ebe8c
Author: Arturo Borrero Gonzalez <[email protected]>
Date:   Wed Jan 25 12:51:08 2017 +0100
payload: explicit network ctx assignment for icmp/icmp6 in special families
Now we generate the right bytecode to restrict this to IPv6:
# nft --debug=netlink add rule inet f i icmpv6 type nd-router-advert
inet f i 
  [ meta load nfproto => reg 1 ]
  [ cmp eq reg 1 0x0000000a ]
  [ payload load 1b @ network header + 6 => reg 1 ]
  [ cmp eq reg 1 0x0000003a ]
  [ payload load 1b @ transport header + 0 => reg 1 ]
  [ cmp eq reg 1 0x00000086 ]

Quoted 1 line

So forget my concern, just remove this mental "postit" note, it's stale ;)

Hm, I wonder why you need this new line in proto_inet_service:

+ PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6),

Florian Westphal

author maintainer

2017-03-22 15:45 UTC #5 lore raw

Quoted 3 lines · Pablo Neira Ayuso

Hm, I wonder why you need this new line in proto_inet_service:
+ PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6),

meta_expr_pctx_update calls proto_find_upper(), without this that returns NULL and proto base is set to 'unknown'.

Pablo Neira Ayuso

maintainer

2017-03-22 16:07 UTC #6 lore raw

Quoted 7 lines · Florian Westphal

Quoted 3 lines · Pablo Neira Ayuso
Hm, I wonder why you need this new line in proto_inet_service:
+ PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6),
meta_expr_pctx_update calls proto_find_upper(), without this that returns NULL and proto base is set to 'unknown'.

Oh right.

Will this still happen if you tell nft to generate the dependency using meta l4proto instead of ip6 nexthdr?

Florian Westphal

author maintainer

2017-03-22 19:24 UTC #7 lore raw

Quoted 6 lines · Pablo Neira Ayuso

Quoted 5 lines · Florian Westphal
Quoted 3 lines · Pablo Neira Ayuso
Hm, I wonder why you need this new line in proto_inet_service:
+ PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6),

Quoted 7 lines

Quoted 2 lines
meta_expr_pctx_update calls proto_find_upper(), without this that returns NULL and proto base is set to 'unknown'.
Oh right.
Will this still happen if you tell nft to generate the dependency using meta l4proto instead of ip6 nexthdr?

Yes, tried with

src/nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert
<cmdline>:1:41-51: Error: conflicting protocols specified: unknown vs. icmpv6

and this patch:

src/proto.c +2 -1

@@ -707,7 +707,7 @@ const struct proto_desc proto_icmp6 = {
 const struct proto_desc proto_ip6 = {
        .name           = "ip6",
        .base           = PROTO_BASE_NETWORK_HDR,
-       .protocol_key   = IP6HDR_NEXTHDR,
+       .protocol_key   = IP6HDR_INVALID,
        .protocols      = {
                PROTO_LINK(IPPROTO_ESP,         &proto_esp),
                PROTO_LINK(IPPROTO_AH,          &proto_ah),

@@ -720,6 +720,7 @@ const struct proto_desc proto_ip6 = {
                PROTO_LINK(IPPROTO_ICMPV6,      &proto_icmp6),
        },
        .templates      = {
+               [IP6HDR_INVALID]        = PROTO_META_TEMPLATE("nfproto", &inet_protocol_type, NFT_META_L4PROTO, 8),
                [IP6HDR_VERSION]        = HDR_BITFIELD("version", &integer_type, 0, 4),
                [IP6HDR_DSCP]           = HDR_BITFIELD("dscp", &dscp_type, 4, 6),
                [IP6HDR_ECN]            = HDR_BITFIELD("ecn", &ecn_type, 10, 2),

Pablo Neira Ayuso

maintainer

2017-03-24 11:50 UTC #8 lore raw

Quoted 26 lines · Florian Westphal

Quoted 6 lines · Pablo Neira Ayuso
Quoted 5 lines · Florian Westphal
Quoted 3 lines · Pablo Neira Ayuso
Hm, I wonder why you need this new line in proto_inet_service:
+ PROTO_LINK(IPPROTO_ICMPV6, &proto_icmp6),
Quoted 7 lines
Quoted 2 lines
meta_expr_pctx_update calls proto_find_upper(), without this that returns NULL and proto base is set to 'unknown'.
Oh right.
Will this still happen if you tell nft to generate the dependency using meta l4proto instead of ip6 nexthdr?
Yes, tried with
src/nft add rule ip6 f i meta l4proto ipv6-icmp icmpv6 type nd-router-advert
<cmdline>:1:41-51: Error: conflicting protocols specified: unknown vs. icmpv6
and this patch:

src/proto.c const struct proto_desc proto_icmp6 = { · line 710

Quoted 5 lines

src/proto.c +1 -1

@@ -707,7 +707,7 @@ const struct proto_desc proto_icmp6 = {
 const struct proto_desc proto_ip6 = {
        .name           = "ip6",
        .base           = PROTO_BASE_NETWORK_HDR,
-       .protocol_key   = IP6HDR_NEXTHDR,
+       .protocol_key   = IP6HDR_INVALID,

In order spots, we just remove this line given IP6HDR_INVALID is zero. I think this may be confusing to newcomers reading the code.

src/proto.c const struct proto_desc proto_ip6 = { · line 723

Quoted 8 lines

src/proto.c +1 -0

        .protocols      = {
                PROTO_LINK(IPPROTO_ESP,         &proto_esp),
                PROTO_LINK(IPPROTO_AH,          &proto_ah),

@@ -720,6 +720,7 @@ const struct proto_desc proto_ip6 = {
                PROTO_LINK(IPPROTO_ICMPV6,      &proto_icmp6),
        },
        .templates      = {
+               [IP6HDR_INVALID]        = PROTO_META_TEMPLATE("nfproto", &inet_protocol_type, NFT_META_L4PROTO, 8),

We can just use NFT_META_L4PROTO all the time, so we use it from IPv4 too, right?

And use:

        [0]     = PROTO_META_TEMPLATE(...)

for consistency.

Florian Westphal

author maintainer

2017-03-24 12:21 UTC #9 lore raw

Quoted 2 lines · Pablo Neira Ayuso

We can just use NFT_META_L4PROTO all the time, so we use it from IPv4 too, right?

Right, we can indeed do that and change ip as well.

Pablo Neira Ayuso

maintainer

2017-03-29 10:13 UTC #10 lore raw

Quoted 5 lines · Florian Westphal

Quoted 2 lines · Pablo Neira Ayuso
We can just use NFT_META_L4PROTO all the time, so we use it from IPv4 too, right?
Right, we can indeed do that and change ip as well.

BTW, I think this problem may be the root cause for this report:

https://bugzilla.netfilter.org/show_bug.cgi?id=1138

Probably these people are just getting that ICMPv6 with some extension header.