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

Message-ID: <CAF=yD-L2YKWLii9dAWGcutYSk+CiJbmmg_OWBrzUNeh_Ci+n_Q@mail.gmail.com>
Date:   Fri, 19 Jan 2018 09:39:47 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Jason Wang <jasowang@...hat.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 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.

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.