netdev - Re: [PATCH net-next 2/2] l2tp: add peer

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]

Date:   Wed, 3 Jan 2018 16:06:28 +0100
From:   Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
To:     Guillaume Nault <g.nault@...halink.fr>
Cc:     James Chapman <jchapman@...alix.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Hangbin Liu <liuhangbin@...il.com>
Subject: Re: [PATCH net-next 2/2] l2tp: add peer_offset parameter

> On Tue, Jan 02, 2018 at 08:28:03PM +0100, Lorenzo Bianconi wrote:
>> Perhaps I am little bit polarized on UABI issue, but I was rethinking
>> about it and maybe removing offset parameter would lead to an
>> interoperability issue for device running L2TPv3 since offset
>> parameter is there and it is not a nope.
>> Please consider this setup:
>> - 2 endpoint running L2TPv3, the first running net-next and the second
>> running 4.14
>> - both endpoint are configured using iproute2 in this way:
>>
>>   - ip l2tp add tunnel local <ip0> remote <ip1> tunnel_id <id0>
>> peer_tunnel_id <id1> udp_sport <p0> udp_dport <p1>
>>   - ip l2tp add tunnel local <ip1> remote <ip0> tunnel_id <id1>
>> peer_tunnel_id <id0> udp_sport <p1> udp_dport <p0>
>>   - ip l2tp add session name l2tp0 tunnel_id <id0> session_id <s0>
>> peer_session_id <s1> offset 8
>>   - ip l2tp add session name l2tp0 tunnel_id <id1> session_id <s1>
>> peer_session_id <s0> offset 8
>>
>> Can we assume offset is never used for L2TPv3?
>>
> That's what I think. You're right worrying about ABI issues. And I
> wouldn't dare proposing such a removal if I had doubts about breaking a
> user setup.
>
> Considering the lack of use cases and the absence of interoperability
> of this feature, I hardly can imagine it being used.
> But it's not only that: the feature has been buggy for years without
> anyone noticing. And this bug wasn't difficult to spot (one just needs
> to look at an L2TPv3 header in a network packet dump).
>
> It's really the combination of these three issues (buggy, no use case
> and not producing valid L2TPv3 frames) that makes me propose a removal.

Hi Guillaume, James,

I agree to remove offset parameter in this case. What about (as
already suggested by James) to take into account possible alignment
issues with previous version of L2TPv3 protocol using 'L2 specific
sublayer'?
I guess, on the kernel side (we will need to patch iproute2 on
userspace side), we need just to properly initialized the 'l2specific'
field to 0 since otherwise we will have the same memleak issue there
if assume we can have l2specific_len != {0,4}.
Moreover does it worth to add some sanity checks in netlink code to
enforce the relation between l2specific_len and l2specific_type? At
the moment there are no guarantee that if l2specific_type is set to
L2TP_L2SPECTYPE_DEFAULT, l2specific_len will be grater or equal than
4.

Regards,
Lorenzo