[PATCH nft] src: allow update of net base w. meta l4proto icmpv6

[PATCH nft] src: allow update of net base w. meta l4proto icmpv6

[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 <twoerner@redhat.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 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)?

diff --git a/src/proto.c b/src/proto.c
index 79e9dbf2b33e..fcdfbe73c735 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -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),
-- 
2.10.2

Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6

[Pablo Neira Ayuso]

On Tue, Mar 21, 2017 at 07:54:37PM +0100, Florian Westphal wrote:
> 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 <twoerner@redhat.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  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)?

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

> diff --git a/src/proto.c b/src/proto.c
> index 79e9dbf2b33e..fcdfbe73c735 100644
> --- a/src/proto.c
> +++ b/src/proto.c
> @@ -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.

Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Mar 21, 2017 at 07:54:37PM +0100, Florian Westphal wrote:
> > 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 <twoerner@redhat.com>
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  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)?
> 
> 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.

> > diff --git a/src/proto.c b/src/proto.c
> > index 79e9dbf2b33e..fcdfbe73c735 100644
> > --- a/src/proto.c
> > +++ b/src/proto.c
> > @@ -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.

Yes, whats the concern with that?

Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6

[Pablo Neira Ayuso]

On Wed, Mar 22, 2017 at 02:44:12PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Tue, Mar 21, 2017 at 07:54:37PM +0100, Florian Westphal wrote:
> > > 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 <twoerner@redhat.com>
> > > Signed-off-by: Florian Westphal <fw@strlen.de>
> > > ---
> > >  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)?
> > 
> > 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.
> 
> > > diff --git a/src/proto.c b/src/proto.c
> > > index 79e9dbf2b33e..fcdfbe73c735 100644
> > > --- a/src/proto.c
> > > +++ b/src/proto.c
> > > @@ -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.
> 
> Yes, whats the concern with that?

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

commit 0011985554e269e1cc8f8e5b41eb9dcd795ebe8c
Author: Arturo Borrero Gonzalez <arturo@debian.org>
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 ;)

Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6

[Pablo Neira Ayuso]

On Wed, Mar 22, 2017 at 04:29:09PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Mar 22, 2017 at 02:44:12PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Tue, Mar 21, 2017 at 07:54:37PM +0100, Florian Westphal wrote:
> > > > 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 <twoerner@redhat.com>
> > > > Signed-off-by: Florian Westphal <fw@strlen.de>
> > > > ---
> > > >  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)?
> > > 
> > > 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.
> > 
> > > > diff --git a/src/proto.c b/src/proto.c
> > > > index 79e9dbf2b33e..fcdfbe73c735 100644
> > > > --- a/src/proto.c
> > > > +++ b/src/proto.c
> > > > @@ -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.
> > 
> > Yes, whats the concern with that?
> 
> Not a problem these days from inet/netdev chains since:
> 
> commit 0011985554e269e1cc8f8e5b41eb9dcd795ebe8c
> Author: Arturo Borrero Gonzalez <arturo@debian.org>
> 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 ;)

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

+         PROTO_LINK(IPPROTO_ICMPV6,      &proto_icmp6),

Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 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'.

Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6

[Pablo Neira Ayuso]

On Wed, Mar 22, 2017 at 04:44:00PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > 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?

Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Mar 22, 2017 at 04:44:00PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > 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?

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:

diff --git a/src/proto.c b/src/proto.c
--- a/src/proto.c
+++ b/src/proto.c
@@ -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),

Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6

[Pablo Neira Ayuso]

On Wed, Mar 22, 2017 at 08:22:52PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Wed, Mar 22, 2017 at 04:44:00PM +0100, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > 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?
> 
> 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:
> 
> diff --git a/src/proto.c b/src/proto.c
> --- a/src/proto.c
> +++ b/src/proto.c
> @@ -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.

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

Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 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.

Re: [PATCH nft] src: allow update of net base w. meta l4proto icmpv6

[Pablo Neira Ayuso]

On Fri, Mar 24, 2017 at 01:21:12PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > 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.