netdev - Re: [PATCH] xfrm: fix inbound ipv4/udp/esp packets to UDPv6 dualstack sockets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHo-Ooy5JB-0R5ZNMmEXaPfGjWKBw8VdXVp0d-XW2CNeO6u34A@mail.gmail.com>
Date:   Wed, 26 Oct 2022 17:42:37 +0900
From:   Maciej Żenczykowski <zenczykowski@...il.com>
To:     Maciej Żenczykowski <maze@...gle.com>
Cc:     Linux Network Development Mailing List <netdev@...r.kernel.org>,
        Sabrina Dubroca <sd@...asysnail.net>,
        Steffen Klassert <steffen.klassert@...unet.com>
Subject: Re: [PATCH] xfrm: fix inbound ipv4/udp/esp packets to UDPv6 dualstack sockets

On Wed, Oct 26, 2022 at 5:32 PM Maciej Żenczykowski
<zenczykowski@...il.com> wrote:
>
> From: Maciej Żenczykowski <maze@...gle.com>
>
> Before Linux v5.8 an AF_INET6 SOCK_DGRAM (udp/udplite) socket
> with SOL_UDP, UDP_ENCAP, UDP_ENCAP_ESPINUDP{,_NON_IKE} enabled
> would just unconditionally use xfrm4_udp_encap_rcv(), afterwards
> such a socket would use the newly added xfrm6_udp_encap_rcv()
> which only handles IPv6 packets.
>
> Cc: Sabrina Dubroca <sd@...asysnail.net>
> Cc: Steffen Klassert <steffen.klassert@...unet.com>
> Fixes: 0146dca70b87 ('xfrm: add support for UDPv6 encapsulation of ESP')
> Signed-off-by: Maciej Żenczykowski <maze@...gle.com>
> ---
>  net/ipv6/xfrm6_input.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c
> index 04cbeefd8982..2d1c75b42709 100644
> --- a/net/ipv6/xfrm6_input.c
> +++ b/net/ipv6/xfrm6_input.c
> @@ -86,6 +86,9 @@ int xfrm6_udp_encap_rcv(struct sock *sk, struct sk_buff *skb)
>         __be32 *udpdata32;
>         __u16 encap_type = up->encap_type;
>
> +       if (skb->protocol == htons(ETH_P_IP))
> +               xfrm4_udp_encap_rcv(sk, skb);
> +
>         /* if this is not encapsulated socket, then just return now */
>         if (!encap_type)
>                 return 1;
> --
> 2.38.0.135.g90850a2211-goog

Does this seem reasonable?

I'll admit that so far I've only tested that the code builds.
However, the current code seems very obviously wrong, as it blindly
assumes (later in the function) that there's an ipv6 header on the
packet...

Our current API for creating these sockets specifies the port, but not
the ip version.
I think it would be beneficial if we could just always use AF_INET6
(and thus dualstack) sockets,
instead of how we currently just always use AF_INET udp sockets.

side note: with nat64 network based packet translation you can
actually end up with ipv6/udp/esp talking to ipv4/udp/esp, etc, and
you might not be able to tell that this is the case from looking at
the IP addresses themselves.