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] [day] [month] [year] [list]

Date:   Mon, 8 Jan 2018 18:27:02 +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 Wed, Jan 03, 2018 at 04:06:28PM +0100, Lorenzo Bianconi wrote:
>> 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 think James was refering to the general architecture of L2TPv3, where
> such issues should be handled by pseudo-wire specific headers. I don't
> think he was talking about implementing arbitrary padding using an
> L2 specific sublayer. None of the standardised headers allow arbitrary
> padding. And implementing our own would make us imcompatible with any
> other implementation.
>
>> 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}.
>>
> That would produce the same frame format as what the 'offset' option
> was supposed to produce (if it did properly initialise its padding
> bits). That is, we'd have an arbitrary number of padding bits inserted
> between the l2-specific header and the l2 frame (L2TP's payload). These
> frames are invalid, and doing that brings us nothing compared to
> keeping the offset option.
>
>> Moreover does it worth to add some sanity checks in netlink code to
>> enforce the relation between l2specific_len and l2specific_type?
>>
> Yes, certainly.
>
>> 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.
>>
> Thanks for pointing this out. That needs to be fixed. We don't support
> anything but the default l2spec layer (or no l2spec layer at all). So
> we should only accept L2TP_L2SPECTYPE_NONE and L2TP_L2SPECTYPE_DEFAULT.
>
> And we shouldn't need to use l2specific_len at all. The default l2spec
> header is always four bytes long, so we don't need to rely on a user
> supplied value. Looking at the code, it seems that invalid usage of
> L2TP_ATTR_L2SPEC_LEN allows leaking memory on the network or sending
> corrupted frames (depending on if its value is too small or too big).
>
> Do you want to take care of this?

Ack, I am working on it.
Regards,

Lorenzo