Date:   Thu, 18 Jan 2018 11:35:50 +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月17日 22:27, Willem de Bruijn wrote:
> 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.

I think that's why we need extra check for dodgy packets here, we used 
to have gso_type verification here but was removed. In the future, 
virtio will support passing sctp offload packet down for sure, so we 
need consider this case.

>   Also, a packet can just as easily spoof an esp packet.
> See also the discussion in the Link above.

Then we probably need check there.

>
> 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;
> +               }
> +       }

This implements subset of function for codes which was removed by the 
commit I mentioned below.

>> 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.

I think we don't even need to care about this consider the evil packet 
should be rare. 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.

>   And now we have two
> pieces of code that need to be hardened against malicious packets.

To me fixing the exist path is a good balance between security and 
performance.

>
> 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 problem is that guests and hosts don't trust each other. Even if one 
packet has already been validated by one side, it must be validated 
again by another side when needed. Doing validation twice is meaningless 
in this case.

>
> 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.

I don't get the point of removing this.

>   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.

I suspect the cost is still noticeable, and consider we may support kind 
of tunnel offload in the future.

We should first assume the correctness of metadata provided by untrusted 
source and validate it before we really want to use it. Validate them 
during entry means we assume they are wrong, then there's no need for 
most of the fields of virtio-net header,

>
> 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.
>

So as I replied before, we probably want to bring back (at least dodgy 
part of 5c7cdf339af5).

>>> 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.

Yes, just no need for embargo.

Thanks

Powered by blists - more mailing lists