Date:   Wed, 17 Jan 2018 09:27:15 -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 Wed, Jan 17, 2018 at 6:54 AM, Jason Wang <jasowang@...hat.com> wrote:
>
>
> On 2018年01月17日 12:33, Willem de Bruijn wrote:
>>
>> On Tue, Jan 16, 2018 at 11:04 PM, Jason Wang <jasowang@...hat.com> wrote:
>>>
>>>
>>> On 2018年01月17日 04:29, Willem de Bruijn wrote:
>>>>
>>>> From: Willem de Bruijn<willemb@...gle.com>
>>>>
>>>> Validate gso packet type and headers on kernel entry. Reuse the info
>>>> gathered by skb_probe_transport_header.
>>>>
>>>> Syzbot found two bugs by passing bad gso packets in packet sockets.
>>>> Untrusted user packets are limited to a small set of gso types in
>>>> virtio_net_hdr_to_skb. But segmentation occurs on packet contents.
>>>> Syzkaller was able to enter gso callbacks that are not hardened
>>>> against untrusted user input.
>>>
>>>
>>> Do this mean there's something missed in exist header check for dodgy
>>> packets?
>>
>> virtio_net_hdr_to_skb checks gso_type, but it does not verify that this
>> type correctly describes the actual packet. Segmentation happens based
>> on packet contents. So a packet was crafted to enter sctp gso, even
>> though no such gso_type exists. This issue is not specific to sctp.
>
>
> So it looks to me we should do it in here in sctp_gso_segment().
>
> if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {
>         /* Packet is from an untrusted source, reset gso_segs. */

No dodgy source can legitimately generate sctp code, so it should not
even get there. Also, a packet can just as easily spoof an esp packet.
See also the discussion in the Link above.

We can address this specific issue in segmentation by placing a check
analogous to skb_validate_dodgy_gso in the network layer. Something
like this (but with ECN option support):

@@ -1258,6 +1258,22 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,

        skb_reset_transport_header(skb);

+       gso_type = skb_shinfo(skb)->gso_type;
+       if (gso_type & SKB_GSO_DODGY) {
+               switch (gso_type & (SKB_GSO_TCPV4 | SKB_GSO_UDP)) {
+               case SKB_GSO_TCPV4:
+                       if (proto != IPPROTO_TCP)
+                               goto out;
+                       break;
+               case SKB_GSO_UDP:
+                       if (proto != IPPROTO_UDP)
+                               goto out;
+                       break;
+               default:
+                       goto out;
+               }
+       }

> And we probably need to recover what has been removed since
> 5c7cdf339af560f980b12eb6b0b5aa5f68ac6658 ("gso: Remove arbitrary checks for
> unsupported GSO").
>
>>
>>>> User packets can also have corrupted headers, tripping up segmentation
>>>> logic that expects sane packets from the trusted protocol stack.
>>>> Hardening all segmentation paths against all bad packets is error
>>>> prone and slows down the common path, so validate on kernel entry.
>>>
>>>
>>> I think evil packets should be rare in common case, so I'm not sure
>>> validate
>>> it on kernel entry is a good choice especially consider we've already had
>>> header check.
>>
>> This just makes that check more strict. Frequency of malicious packets is
>> not really relevant if a single bad packet can cause damage.
>
>
> We try hard to avoid flow dissector since its overhead is obvious. But looks
> like this patch did it unconditionally, and even for non gso packet.
>
>> The alternative to validate on kernel entry is to harden the entire
>> segmentation
>> layer and lower part of the stack. That is much harder to get right and
>> not
>> necessarily cheaper.
>
>
> For performance reason. I think we should delay the check or segmentation as
> much as possible until it was really needed.

Going through segmentation is probably as expensive as flow dissector,
if not more so because of the indirect branches. And now we have two
pieces of code that need to be hardened against malicious packets.

But I did overlook the guest to guest communication, with virtual devices
that can set NETIF_F_GSO_ROBUST and so do not enter the stack. That
means that one guest can send misrepresented packets to another. Is that
preferable to absorbing the cost of validation?

The current patch does not actually deprecate the path through the
segmentation layer. So it will indeed add overhead. We would have to
remove the DODGY logic in net-next. One additional possible optimization
is to switch to flow_keys_buf_dissector, which does not dissect as deeply.
I did that in an earlier patch; it should be sufficient for this purpose.

A third option is to apply the above inet_gso_segment fix in net, then
add flow dissection at the source in net-next together with removal of
dodgy checks in the stack. Then that can be benchmarked properly.

>>
>> As a matter of fact, it incurs a cost on all packets, including the common
>> case generated by the protocol stack.
>
>
> Btw, this looks could be triggered from guest. So it looks at least a DOS
> form guest to host which should be treated as CVE?

Good point. The report was public, so I focused on getting it fixed first.
You mean just to request a CVE? I'll do that that once we have a fix.

Powered by blists - more mailing lists