netdev - Re: [PATCH net] l2tp: Allow duplicate session creation with UDP

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: <20200116131223.GB4028@jackdaw>
Date:   Thu, 16 Jan 2020 13:12:24 +0000
From:   Tom Parkin <tparkin@...alix.com>
To:     Guillaume Nault <gnault@...hat.com>
Cc:     Ridge Kennedy <ridge.kennedy@...iedtelesis.co.nz>,
        netdev@...r.kernel.org
Subject: Re: [PATCH net] l2tp: Allow duplicate session creation with UDP

On  Thu, Jan 16, 2020 at 13:38:54 +0100, Guillaume Nault wrote:
> Well, I think we have a more fundamental problem here. By adding
> L2TPoUDP sessions to the global list, we allow cross-talks with L2TPoIP
> sessions. That is, if we have an L2TPv3 session X running over UDP and
> we receive an L2TP_IP packet targetted at session ID X, then
> l2tp_session_get() will return the L2TP_UDP session to l2tp_ip_recv().
> 
> I guess l2tp_session_get() should be dropped and l2tp_ip_recv() should
> look up the session in the context of its socket, like in the UDP case.
> 
> But for the moment, what about just not adding L2TP_UDP sessions to the
> global list? That should fix both your problem and the L2TP_UDP/L2TP_IP
> cross-talks.
> 
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index f82ea12bac37..f70fea8d093d 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -316,7 +316,7 @@ int l2tp_session_register(struct l2tp_session *session,
>  			goto err_tlock;
>  		}
>  
> -	if (tunnel->version == L2TP_HDR_VER_3) {
> +	if (tunnel->encap == L2TP_ENCAPTYPE_IP) {
>  		pn = l2tp_pernet(tunnel->l2tp_net);
>  		g_head = l2tp_session_id_hash_2(pn, session->session_id);
>  
> @@ -1587,8 +1587,8 @@ void __l2tp_session_unhash(struct l2tp_session *session)
>  		hlist_del_init(&session->hlist);
>  		write_unlock_bh(&tunnel->hlist_lock);
>  
> -		/* For L2TPv3 we have a per-net hash: remove from there, too */
> -		if (tunnel->version != L2TP_HDR_VER_2) {
> +		/* For IP encap we have a per-net hash: remove from there, too */
> +		if (tunnel->encap == L2TP_ENCAPTYPE_IP) {
>  			struct l2tp_net *pn = l2tp_pernet(tunnel->l2tp_net);
>  			spin_lock_bh(&pn->l2tp_session_hlist_lock);
>  			hlist_del_init_rcu(&session->global_hlist);
>

I agree with you about the possibility for cross-talk, and I would
welcome l2tp_ip/ip6 doing more validation.  But I don't think we should
ditch the global list.

As per the RFC, for L2TPv3 the session ID should be a unique
identifier for the LCCE.  So it's reasonable that the kernel should
enforce that when registering sessions.

When you're referring to cross-talk, I wonder whether you have in mind
normal operation or malicious intent?  I suppose it would be possible
for someone to craft session data packets in order to disrupt a
session.

For normal operation, you just need to get the wrong packet on the
wrong socket to run into trouble of course.  In such a situation
having a unique session ID for v3 helps you to determine that
something has gone wrong, which is what the UDP encap recv path does
if the session data packet's session ID isn't found in the context of
the socket that receives it.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)