netdev - Re: [PATCH net-next 2/3] l2tp: configure l2specific_len according to l2specific

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]

Message-ID: <CAJ0CqmVgy=nLOexJrX_gRzE4tFJfLC+nodqPkc+JCNheHf1cJQ@mail.gmail.com>
Date:   Wed, 10 Jan 2018 19:06:32 +0100
From:   Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
To:     Guillaume Nault <g.nault@...halink.fr>
Cc:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        James Chapman <jchapman@...alix.com>
Subject: Re: [PATCH net-next 2/3] l2tp: configure l2specific_len according to l2specific_type

> On Wed, Jan 10, 2018 at 06:20:41PM +0100, Lorenzo Bianconi wrote:
>> Set l2specific_len according to the L2-Specific Sublayer type specified
>> by the userspace and not rely on a user supplied value for sublayer len
>> since invalid usage of L2TP_ATTR_L2SPEC_LEN allows leaking memory on the
>> network and sending corrupted frames.
>> Moreover add a sanity check on l2specific_type provided by userspace
>>
>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
>> ---
>>  net/l2tp/l2tp_netlink.c | 21 ++++++++++++++++-----
>>  1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
>> index 48b5bf30ec50..404e5482c4e7 100644
>> --- a/net/l2tp/l2tp_netlink.c
>> +++ b/net/l2tp/l2tp_netlink.c
>> @@ -550,13 +550,24 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
>>               if (info->attrs[L2TP_ATTR_DATA_SEQ])
>>                       cfg.data_seq = nla_get_u8(info->attrs[L2TP_ATTR_DATA_SEQ]);
>>
>> -             cfg.l2specific_type = L2TP_L2SPECTYPE_DEFAULT;
>> -             if (info->attrs[L2TP_ATTR_L2SPEC_TYPE])
>> +             if (info->attrs[L2TP_ATTR_L2SPEC_TYPE]) {
>>                       cfg.l2specific_type = nla_get_u8(info->attrs[L2TP_ATTR_L2SPEC_TYPE]);
>>
>> -             cfg.l2specific_len = 4;
>> -             if (info->attrs[L2TP_ATTR_L2SPEC_LEN])
>> -                     cfg.l2specific_len = nla_get_u8(info->attrs[L2TP_ATTR_L2SPEC_LEN]);
>> +                     switch (cfg.l2specific_type) {
>> +                     case L2TP_L2SPECTYPE_DEFAULT:
>> +                             cfg.l2specific_len = 4;
>> +                             break;
>> +                     case L2TP_L2SPECTYPE_NONE:
>> +                             cfg.l2specific_len = 0;
>> +                             break;
>> +                     default:
>> +                             ret = -EINVAL;
>> +                             goto out_tunnel;
>> +                     }
>> +             } else {
>> +                     cfg.l2specific_type = L2TP_L2SPECTYPE_DEFAULT;
>> +                     cfg.l2specific_len = 4;
>> +             }
>>
>>               if (info->attrs[L2TP_ATTR_COOKIE]) {
>>                       u16 len = nla_len(info->attrs[L2TP_ATTR_COOKIE]);
>>
> I think we can go one step further and remove .l2specific_len from
> struct l2tp_session and struct l2tp_session_cfg. We never need this
> information. Then we can start ignoring the L2TP_ATTR_L2SPEC_LEN
> attribute just like we've done with L2TP_ATTR_OFFSET.

Hi Guillaume,

I was thinking about it, let's way for some feedbacks and then I can
respin a v2.
Regards,

Lorenzo