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]

Message-ID: <20180103163525.GF1402@alphalink.fr>
Date:   Wed, 3 Jan 2018 17:35:25 +0100
From:   Guillaume Nault <g.nault@...halink.fr>
To:     Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
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?