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

[Guillaume Nault <gnault@redhat.com>]

On Thu, Jan 16, 2020 at 01:12:24PM +0000, Tom Parkin wrote:
> 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.
> 
I had never thought that the session ID could have global significance
in L2TPv3, but maybe that's because my experience is mostly about
L2TPv2. I haven't read RFC 3931 in detail, but I can't see how
restricting the scope of sessions to their parent tunnel would conflict
with the RFC.

> 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.
> 
What makes me uneasy is that, as soon as the l2tp_ip or l2tp_ip6 module
is loaded, a peer can reach whatever L2TPv3 session exists on the host
just by sending an L2TP_IP packet to it.
I don't know how practical it is to exploit this fact, but it looks
like it's asking for trouble.

> 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.
Unique global session IDs might help troubleshooting, but they also
break some use cases, as reported by Ridge. At some point, we'll have
to make a choice, or even add a knob if necessary.