netdev - Re: [PATCH] l2tp: Support several sockets with same IP/port quadruple

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: <66998255-7078-8d4b-6efa-fa7b0751176e@katalix.com>
Date: Tue, 7 May 2024 09:06:35 +0100
From: James Chapman <jchapman@...alix.com>
To: Samuel Thibault <samuel.thibault@...-lyon.org>,
 Tom Parkin <tparkin@...alix.com>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 netdev@...r.kernel.org
Subject: Re: [PATCH] l2tp: Support several sockets with same IP/port quadruple

Hi Samuel,

On 06/05/2024 22:44, Samuel Thibault wrote:
> Hello,
>
> James Chapman, le ven. 03 mai 2024 12:36:14 +0100, a ecrit:
>
>>> +			/* We are receiving trafic for another tunnel, probably
>>> +			 * because we have several tunnels between the same
>>> +			 * IP/port quadruple, look it up.
>>> +			 */
>>> +			struct l2tp_tunnel *alt_tunnel;
>>> +
>>> +			alt_tunnel = l2tp_tunnel_get(tunnel->l2tp_net, tunnel_id);
>> This misses a check that alt_tunnel's protocol version matches the header.
>> Move the existing header version check to after this fragment?
> We need to check the version before getting the tunnel id, which we need
> to look up the struct l2tp_tunnel :)
I was referring to the following code fragment which is before your change:

 >    version = hdrflags & L2TP_HDR_VER_MASK;
 >    if (version != tunnel->version) {
 >        pr_debug_ratelimited("%s: recv protocol version mismatch: got 
%d expected %d\n",
 >                     tunnel->name, version, tunnel->version);
 >        goto invalid;
 >    }

The tunnel->version check should now be done after the tunnel pointer is 
possibly modified by your code.

Also, if the tunnel pointer from sk_user_data isn't trusted due to 
5-tuple aliasing, l2tp_udp_recv_core should compare with the local 
'version' variable, not tunnel->version, when parsing the L2TP IDs e.g.:

 >    if (version == L2TP_HDR_VER_2) {
 >        /* If length is present, skip it */

otherwise, L2TPv2 socket aliasing will still not work properly if one or 
more L2TPv3 sockets also alias L2TPv2 sockets, even if there is no 
L2TPv3 traffic.