netdev - Re: [PATCH net] net: validate untrusted gso packets

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: <57040eaf-cfa8-9c0a-efed-e0f171cb7251@redhat.com>
Date:   Mon, 22 Jan 2018 10:44:30 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net] net: validate untrusted gso packets



On 2018年01月19日 22:39, Willem de Bruijn wrote:
> On Fri, Jan 19, 2018 at 3:19 AM, Jason Wang <jasowang@...hat.com> wrote:
>>
>> On 2018年01月19日 08:53, Willem de Bruijn wrote:
>>>>>> And what you propose here is just a very small subset of the
>>>>>> necessary checking, more comes at gso header checking. So even if we
>>>>>> care
>>>>>> performance, it only help for some specific case.
>>>>> It also fixed the bug that Eric sent a separate patch for, as that did
>>>>> not dissect as a valid TCP packet, either.
>>>> I may miss something but how did this patch protects an evil thoff?
>>> Actually, it blocked that specific reproducer because the ip protocol
>>> did not match.
>>
>> I see.
>>
>>> I think that __skb_flow_dissect_tcp should return a boolean, causing
>>> dissection return FLOW_DISSECT_RET_OUT_BAD if the tcph is bad.
>>> That would be needed to really catch it with flow dissection at the
>>> source.
>>
>> Just sanitize transport to offset_hint (0) in the case of tun.
> That is the current approach in skb_probe_transport_header, but it
> opens us up to parsing of garbage headers later in the stack when
> unconditionally reading tcp_hdrlen. The qdisc_pkt_len_init case is one
> such example. Seems better to leave transport header unset in such
> cases and qualify all access to the headers if DODGY.
>
>> It looks to
>> me flow dissector will return FLOW_DISSECT_RET_OUT_BAD too if it can't
>> recognize the protocol. We can't differ the real failure from unrecognized
>> protocol. (or change the return from bool to int).
> Unrecognized protocol should imply failure. virtio_net_hdr_to_skb accepts
> a whitelist of protocols, all of which the flow dissector can verify.

Yes and only for gso packets.

>
> Another argument for early validation: we cannot currently differentiate
> tunnel headers inserted by the tun/pf_packet/xen user from those
> generated by the stack if both are present.
>
> The kernel currently does not define tunnel VIRTIO_NET_HDR_GSO
> types, so should drop packets with tunnel headers. Tunnel gso handlers
> indeed do drop these if skb->encapsulation is not set.
>
> But if a packet travels through a tunnel device and the bit is set, at
> segmentation time we cannot distinguish trusted stack headers from
> possibly malformed user supplied ones.

Right.

Thanks