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: <alpine.DEB.2.21.2001171016080.9038@ridgek-dl.ws.atlnz.lc>
Date:   Fri, 17 Jan 2020 10:26:09 +1300 (NZDT)
From:   Ridge Kennedy <ridgek@...iedtelesis.co.nz>
To:     Guillaume Nault <gnault@...hat.com>
cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net] l2tp: Allow duplicate session creation with UDP



On Thu, 16 Jan 2020, Guillaume Nault wrote:

> 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.

I did consider not adding the L2TP_UDP sessions to the global list, but 
that change looked a little more involved as the netlink interface was 
also making use of the list to lookup sessions by ifname. At a minimum
it looks like this would require rework of l2tp_session_get_by_ifname().