[net] netfilter: conntrack: fix ct-state for ICMPv6 Multicast Router Discovery

So far Multicast Router Advertisements and Multicast Router
Solicitations from the Multicast Router Discovery protocol (RFC4286)
would be marked as INVALID for IPv6, even if they are in fact intact
and adhering to RFC4286.

This broke MRA reception and by that multicast reception on
IPv6 multicast routers in a Proxmox managed setup, where Proxmox
would install a rule like "-m conntrack --ctstate INVALID -j DROP"
at the top of the FORWARD chain with br-nf-call-ip6tables enabled
by default.

Similar to as it's done for MLDv1, MLDv2 and IPv6 Neighbor Discovery
already, fix this issue by excluding MRD from connection tracking
handling as MRD always uses predefined multicast destinations
for its messages, too. This changes the ct-state for ICMPv6 MRD messages
from INVALID to UNTRACKED.

This issue was found and fixed with the help of the mrdisc tool
(https://github.com/troglobit/mrdisc).

Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---
 include/uapi/linux/icmpv6.h               | 1 +
 net/netfilter/nf_conntrack_proto_icmpv6.c | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

On Wed, Mar 06, 2024 at 03:18:04PM +0100, Linus Lüssing wrote:
> So far Multicast Router Advertisements and Multicast Router
> Solicitations from the Multicast Router Discovery protocol (RFC4286)
> would be marked as INVALID for IPv6, even if they are in fact intact
> and adhering to RFC4286.
> 
> This broke MRA reception and by that multicast reception on
> IPv6 multicast routers in a Proxmox managed setup, where Proxmox
> would install a rule like "-m conntrack --ctstate INVALID -j DROP"
> at the top of the FORWARD chain with br-nf-call-ip6tables enabled
> by default.
> 
> Similar to as it's done for MLDv1, MLDv2 and IPv6 Neighbor Discovery
> already, fix this issue by excluding MRD from connection tracking
> handling as MRD always uses predefined multicast destinations
> for its messages, too. This changes the ct-state for ICMPv6 MRD messages
> from INVALID to UNTRACKED.
> 
> This issue was found and fixed with the help of the mrdisc tool
> (https://github.com/troglobit/mrdisc).
> 
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>

Hi Linus,

this appears to be a fix and as such I think it warrants a Fixes tag.
You should be able to just add it to this thread if no other changes
are required - no need for a v2 just to address this.

...

On Thu, Mar 07, 2024 at 10:12:54AM +0000, Simon Horman wrote:
> Hi Linus,
> 
> this appears to be a fix and as such I think it warrants a Fixes tag.
> You should be able to just add it to this thread if no other changes
> are required - no need for a v2 just to address this.
> 
> ...

Hi Simon,

From reading the code and git logs I suspect this
commit, which introduced icmpv6_error():

  9fb9cbb1082d [NETFILTER]: Add nf_conntrack subsystem.

  (introduced in: Linux v2.6.15 / 2006)

Unfortunately, I was only able to reproduce it in practice
on a Debian 5 / Linux 2.6.26 in a VM so far.

I could boot a Debian 4 + Linux 2.6.15, but wasn't able to
insert conntrack rules with ip6tables there though, even
with some iptables + kernel rebuilds/reconfigure attempts.



Also this related fix introduced in v2.6.29 should hint to the
age of this issue:

  3f9007135c1d netfilter: nf_conntrack_ipv6: don't track ICMPv6 negotiation message

Which only picked/fixed a few ICMPv6 types but not ICMPv6 MRD
though.


tl;dr: for me this would be ok, if it were ok for others, too, that I
couldn't fully bisect to it in practice... :

Fixes: 9fb9cbb1082d ("[NETFILTER]: Add nf_conntrack subsystem.")

Linus Lüssing <linus.luessing@c0d3.blue> wrote:
> Also this related fix introduced in v2.6.29 should hint to the
> age of this issue:
> 
>   3f9007135c1d netfilter: nf_conntrack_ipv6: don't track ICMPv6 negotiation message
> 
> Which only picked/fixed a few ICMPv6 types but not ICMPv6 MRD
> though.
> 
> tl;dr: for me this would be ok, if it were ok for others, too, that I
> couldn't fully bisect to it in practice... :
> 
> Fixes: 9fb9cbb1082d ("[NETFILTER]: Add nf_conntrack subsystem.")

Thats fine, its clear that this is isn't a regression this way.

On Wed, Mar 06, 2024 at 03:18:04PM +0100, Linus Lüssing wrote:
> So far Multicast Router Advertisements and Multicast Router
> Solicitations from the Multicast Router Discovery protocol (RFC4286)
> would be marked as INVALID for IPv6, even if they are in fact intact
> and adhering to RFC4286.

There is also RFC4890 which specifies that also acts as multicast
routers need to process these message on their interfaces.

> This broke MRA reception and by that multicast reception on
> IPv6 multicast routers in a Proxmox managed setup, where Proxmox
> would install a rule like "-m conntrack --ctstate INVALID -j DROP"
> at the top of the FORWARD chain with br-nf-call-ip6tables enabled
> by default.
> 
> Similar to as it's done for MLDv1, MLDv2 and IPv6 Neighbor Discovery
> already, fix this issue by excluding MRD from connection tracking
> handling as MRD always uses predefined multicast destinations
> for its messages, too. This changes the ct-state for ICMPv6 MRD messages
> from INVALID to UNTRACKED.

An explicit rule will be still needed to accept this traffic, assuming
default policy to drop. I think that the issue is likely that this
"drop invalid rules" is the at the very beginning of the ruleset.

Anyway, turning this from invalid to untracked seems sensible to me.
Users will still have to explicitly allow for this in their ruleset
assuming default policy to drop.

I am going to include your Fixes: tag and pass up this patch upstream.

Thanks.

> This issue was found and fixed with the help of the mrdisc tool
> (https://github.com/troglobit/mrdisc).
> 
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
> ---
>  include/uapi/linux/icmpv6.h               | 1 +
>  net/netfilter/nf_conntrack_proto_icmpv6.c | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/icmpv6.h b/include/uapi/linux/icmpv6.h
> index ecaece3af38d..4eaab89e2856 100644
> --- a/include/uapi/linux/icmpv6.h
> +++ b/include/uapi/linux/icmpv6.h
> @@ -112,6 +112,7 @@ struct icmp6hdr {
>  #define ICMPV6_MOBILE_PREFIX_ADV	147
>  
>  #define ICMPV6_MRDISC_ADV		151
> +#define ICMPV6_MRDISC_SOL		152
>  
>  #define ICMPV6_MSG_MAX          255
>  
> diff --git a/net/netfilter/nf_conntrack_proto_icmpv6.c b/net/netfilter/nf_conntrack_proto_icmpv6.c
> index 1020d67600a9..327b8059025d 100644
> --- a/net/netfilter/nf_conntrack_proto_icmpv6.c
> +++ b/net/netfilter/nf_conntrack_proto_icmpv6.c
> @@ -62,7 +62,9 @@ static const u_int8_t noct_valid_new[] = {
>  	[NDISC_ROUTER_ADVERTISEMENT - 130] = 1,
>  	[NDISC_NEIGHBOUR_SOLICITATION - 130] = 1,
>  	[NDISC_NEIGHBOUR_ADVERTISEMENT - 130] = 1,
> -	[ICMPV6_MLD2_REPORT - 130] = 1
> +	[ICMPV6_MLD2_REPORT - 130] = 1,
> +	[ICMPV6_MRDISC_ADV - 130] = 1,
> +	[ICMPV6_MRDISC_SOL - 130] = 1
>  };
>  
>  bool nf_conntrack_invert_icmpv6_tuple(struct nf_conntrack_tuple *tuple,
> -- 
> 2.43.0
>

On Thu, Mar 21, 2024 at 09:52:27PM +0100, Pablo Neira Ayuso wrote:
> [...]
> I am going to include your Fixes: tag and pass up this patch upstream.
> 
> Thanks.

Thanks, sounds good to me! (in case my final OK was still expected/needed)