Message-ID: <CAF=yD-K7icwrstyAAaeR4OZUshxQLG2QQHsDYvHQQHBuL_h_Og@mail.gmail.com>
Date:   Thu, 18 Jan 2018 00:09:31 -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

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

Commit 5c7cdf339af5 ("gso: Remove arbitrary checks for unsupported
GSO") only compares gso_type against a list of allowed gso types.

It did not compare gso_type against the actual packet contents, so
would not have protected against this vulnerability.

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

Then the sctp handler will first need to be fixed to not crash on bad packets.
And every other handler for which no virtio gso type exists.

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

Adding a check in every gso handler that does not expect packets
with SKB_GSO_DODGY source seems backwards to me.

A packet with gso_type SKB_GSO_TCPV4 should never be
delivered to an sctp handler, say. We must check before passing
to a handler whether the packet matches the callback.

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

No, as I explain above, it performs a different check.

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

How does frequency matter when a single packet can crash a host?

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

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

Ack, agreed that guests need to have defensive coding of themselves.
Then it's fine to pass packets to guests where the gso_type does not
match the contents.

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

If you always need to use the data, then you always validate. In which case
you want to validate early, as there will be less vulnerable code before
validation.

But I see what I think is your point: in guest to guest communication we
do not need to validate at all, so early validation adds unnecessary cost
for this important use case. That's a fair argument for validating later and
only when needed (i.e., a path without NETIF_F_GSO_ROBUST).

Powered by blists - more mailing lists