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: <20200116123854.GA23974@linux.home>
Date:   Thu, 16 Jan 2020 13:38:54 +0100
From:   Guillaume Nault <gnault@...hat.com>
To:     Ridge Kennedy <ridge.kennedy@...iedtelesis.co.nz>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net] l2tp: Allow duplicate session creation with UDP

On Thu, Jan 16, 2020 at 11:34:47AM +1300, Ridge Kennedy wrote:
> In the past it was possible to create multiple L2TPv3 sessions with the
> same session id as long as the sessions belonged to different tunnels.
> The resulting sessions had issues when used with IP encapsulated tunnels,
> but worked fine with UDP encapsulated ones. Some applications began to
> rely on this behaviour to avoid having to negotiate unique session ids.
> 
> Some time ago a change was made to require session ids to be unique across
> all tunnels, breaking the applications making use of this "feature".
> 
> This change relaxes the duplicate session id check to allow duplicates
> if both of the colliding sessions belong to UDP encapsulated tunnels.
> 
> Fixes: dbdbc73b4478 ("l2tp: fix duplicate session creation")
> Signed-off-by: Ridge Kennedy <ridge.kennedy@...iedtelesis.co.nz>
> ---
>  net/l2tp/l2tp_core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index f82ea12bac37..0cc86227c618 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -323,7 +323,9 @@ int l2tp_session_register(struct l2tp_session *session,
>  		spin_lock_bh(&pn->l2tp_session_hlist_lock);
>  
>  		hlist_for_each_entry(session_walk, g_head, global_hlist)
> -			if (session_walk->session_id == session->session_id) {
> +			if (session_walk->session_id == session->session_id &&
> +			    (session_walk->tunnel->encap == L2TP_ENCAPTYPE_IP ||
> +			     tunnel->encap == L2TP_ENCAPTYPE_IP)) {
>  				err = -EEXIST;
>  				goto err_tlock_pnlock;
>  			}
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);