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

[Jason Wang <jasowang@redhat.com>]

On 2018年01月18日 13:09, Willem de Bruijn wrote:
>>>    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.

That's the case for trusted source. For dodgy source, we can't assume that.

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

[...]

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

I mean consider we had fix the crash, we don't care how expensive do we 
spot this.

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

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

Yes, and looking at Herbert's commit log for 576a30eb6453 ("[NET]: Added 
GSO header verification"). It was intended do the verification just 
before gso.

Thanks