[PATCH net] gso: validate gso_type if SKB_GSO_DODGY

[PATCH net] gso: validate gso_type if SKB_GSO_DODGY

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Validate gso_type of untrusted SKB_GSO_DODGY packets during
segmentation.

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.

Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
Link: http://lkml.kernel.org/r/<001a1137452496ffc305617e5fe0@google.com>
Reported-by: syzbot+fee64147a25aecd48055@syzkaller.appspotmail.com
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/ipv4/af_inet.c     | 21 +++++++++++++++++++++
 net/ipv6/ip6_offload.c | 23 ++++++++++++++++++++++-
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index f00499a46927..d5a36827f7b1 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1220,6 +1220,25 @@ int inet_sk_rebuild_header(struct sock *sk)
 }
 EXPORT_SYMBOL(inet_sk_rebuild_header);
 
+static bool inet_gso_validate_dodgy(struct sk_buff *skb, int ipproto)
+{
+	unsigned int gso_type = skb_shinfo(skb)->gso_type;
+
+	if (gso_type & SKB_GSO_DODGY) {
+		switch (gso_type & ~SKB_GSO_DODGY) {
+		case SKB_GSO_TCPV4:
+		case SKB_GSO_TCPV4 | SKB_GSO_TCP_ECN:
+			return ipproto == IPPROTO_TCP;
+		case SKB_GSO_UDP:
+			return ipproto == IPPROTO_UDP;
+		default:
+			return false;
+		}
+	}
+
+	return true;
+}
+
 struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 				 netdev_features_t features)
 {
@@ -1245,6 +1264,8 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
 
 	id = ntohs(iph->id);
 	proto = iph->protocol;
+	if (!inet_gso_validate_dodgy(skb, proto))
+		goto out;
 
 	/* Warning: after this point, iph might be no longer valid */
 	if (unlikely(!pskb_may_pull(skb, ihl)))
diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
index 4a87f9428ca5..662e16548104 100644
--- a/net/ipv6/ip6_offload.c
+++ b/net/ipv6/ip6_offload.c
@@ -55,6 +55,25 @@ static int ipv6_gso_pull_exthdrs(struct sk_buff *skb, int proto)
 	return proto;
 }
 
+static bool ipv6_gso_validate_dodgy(struct sk_buff *skb, int ipproto)
+{
+	unsigned int gso_type = skb_shinfo(skb)->gso_type;
+
+	if (gso_type & SKB_GSO_DODGY) {
+		switch (gso_type & ~SKB_GSO_DODGY) {
+		case SKB_GSO_TCPV6:
+		case SKB_GSO_TCPV6 | SKB_GSO_TCP_ECN:
+			return ipproto == IPPROTO_TCP;
+		case SKB_GSO_UDP:
+			return ipproto == IPPROTO_UDP;
+		default:
+			return false;
+		}
+	}
+
+	return true;
+}
+
 static struct sk_buff *ipv6_gso_segment(struct sk_buff *skb,
 	netdev_features_t features)
 {
@@ -82,10 +101,12 @@ static struct sk_buff *ipv6_gso_segment(struct sk_buff *skb,
 
 	ipv6h = ipv6_hdr(skb);
 	__skb_pull(skb, sizeof(*ipv6h));
-	segs = ERR_PTR(-EPROTONOSUPPORT);
 
 	proto = ipv6_gso_pull_exthdrs(skb, ipv6h->nexthdr);
+	if (!ipv6_gso_validate_dodgy(skb, proto))
+		goto out;
 
+	segs = ERR_PTR(-EPROTONOSUPPORT);
 	if (skb->encapsulation &&
 	    skb_shinfo(skb)->gso_type & (SKB_GSO_IPXIP4 | SKB_GSO_IPXIP6))
 		udpfrag = proto == IPPROTO_UDP && encap;
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH net] gso: validate gso_type if SKB_GSO_DODGY

[Jason Wang]

On 2018年01月18日 07:11, Willem de Bruijn wrote:
> From: Willem de Bruijn<willemb@google.com>
>
> Validate gso_type of untrusted SKB_GSO_DODGY packets during
> segmentation.
>
> 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.
>
> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")

This commit is suspicious, I guess it should be 5c7cdf339af5 ("gso: 
Remove arbitrary checks for unsupported GSO")

> Link:http://lkml.kernel.org/r/<001a1137452496ffc305617e5fe0@google.com>
> Reported-by:syzbot+fee64147a25aecd48055@syzkaller.appspotmail.com
> Signed-off-by: Willem de Bruijn<willemb@google.com>
> ---
>   net/ipv4/af_inet.c     | 21 +++++++++++++++++++++
>   net/ipv6/ip6_offload.c | 23 ++++++++++++++++++++++-
>   2 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index f00499a46927..d5a36827f7b1 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1220,6 +1220,25 @@ int inet_sk_rebuild_header(struct sock *sk)
>   }
>   EXPORT_SYMBOL(inet_sk_rebuild_header);
>   
> +static bool inet_gso_validate_dodgy(struct sk_buff *skb, int ipproto)
> +{
> +	unsigned int gso_type = skb_shinfo(skb)->gso_type;
> +
> +	if (gso_type & SKB_GSO_DODGY) {
> +		switch (gso_type & ~SKB_GSO_DODGY) {
> +		case SKB_GSO_TCPV4:
> +		case SKB_GSO_TCPV4 | SKB_GSO_TCP_ECN:
> +			return ipproto == IPPROTO_TCP;
> +		case SKB_GSO_UDP:
> +			return ipproto == IPPROTO_UDP;
> +		default:
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}

This seems more strict than what was removed by 5c7cdf339af5. Any reason 
for this? I'm asking since this probably work for virito-net but I'm not 
sure it works for other sources.

Thanks

Re: [PATCH net] gso: validate gso_type if SKB_GSO_DODGY

[Willem de Bruijn]

On Wed, Jan 17, 2018 at 10:48 PM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2018年01月18日 07:11, Willem de Bruijn wrote:
>>
>> From: Willem de Bruijn<willemb@google.com>
>>
>> Validate gso_type of untrusted SKB_GSO_DODGY packets during
>> segmentation.
>>
>> 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.
>>
>> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
>
>
> This commit is suspicious, I guess it should be 5c7cdf339af5 ("gso: Remove
> arbitrary checks for unsupported GSO")

The specific SCTP path was introduced with commit 90017accff61 ("sctp:
add GSO support"). But the main issue that packets can be delivered to
gso handlers different from their gso_type goes back further.

The commit you reference is actually older than the sctp gso patch, so
it makes sense that it did not have a check in the sctp_gso_segment.

I still think that we should check in inet_gso_segment when we have
the proto, instead of in each {tcp, sctp, udp, esp, ...} handler having
a check of the form.

      !(type & (SKB_GSO_TCPV4 |
        SKB_GSO_TCPV6))))

Note that the above commit also only had these checks in the TSO
branch

  if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {

but the case where this condition fails has to be protected just as
well, so a revert of that patch + new tests in all handlers added
since is not sufficient.

>> Link:http://lkml.kernel.org/r/<001a1137452496ffc305617e5fe0@google.com>
>> Reported-by:syzbot+fee64147a25aecd48055@syzkaller.appspotmail.com
>> Signed-off-by: Willem de Bruijn<willemb@google.com>
>> ---
>>   net/ipv4/af_inet.c     | 21 +++++++++++++++++++++
>>   net/ipv6/ip6_offload.c | 23 ++++++++++++++++++++++-
>>   2 files changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
>> index f00499a46927..d5a36827f7b1 100644
>> --- a/net/ipv4/af_inet.c
>> +++ b/net/ipv4/af_inet.c
>> @@ -1220,6 +1220,25 @@ int inet_sk_rebuild_header(struct sock *sk)
>>   }
>>   EXPORT_SYMBOL(inet_sk_rebuild_header);
>>   +static bool inet_gso_validate_dodgy(struct sk_buff *skb, int ipproto)
>> +{
>> +       unsigned int gso_type = skb_shinfo(skb)->gso_type;
>> +
>> +       if (gso_type & SKB_GSO_DODGY) {
>> +               switch (gso_type & ~SKB_GSO_DODGY) {
>> +               case SKB_GSO_TCPV4:
>> +               case SKB_GSO_TCPV4 | SKB_GSO_TCP_ECN:
>> +                       return ipproto == IPPROTO_TCP;
>> +               case SKB_GSO_UDP:
>> +                       return ipproto == IPPROTO_UDP;
>> +               default:
>> +                       return false;
>> +               }
>> +       }
>> +
>> +       return true;
>> +}
>
>
> This seems more strict than what was removed by 5c7cdf339af5. Any reason for
> this? I'm asking since this probably work for virito-net but I'm not sure it
> works for other sources.

All sources are limited to the same VIRTIO_NET_HDR_GSO_.. types that
virtio_net_hdr_to_skb supports, as far as I know.

Re: [PATCH net] gso: validate gso_type if SKB_GSO_DODGY

[Willem de Bruijn]

On Thu, Jan 18, 2018 at 12:20 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Wed, Jan 17, 2018 at 10:48 PM, Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>> On 2018年01月18日 07:11, Willem de Bruijn wrote:
>>>
>>> From: Willem de Bruijn<willemb@google.com>
>>>
>>> Validate gso_type of untrusted SKB_GSO_DODGY packets during
>>> segmentation.
>>>
>>> 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.
>>>
>>> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
>>
>>
>> This commit is suspicious, I guess it should be 5c7cdf339af5 ("gso: Remove
>> arbitrary checks for unsupported GSO")
>
> The specific SCTP path was introduced with commit 90017accff61 ("sctp:
> add GSO support"). But the main issue that packets can be delivered to
> gso handlers different from their gso_type goes back further.
>
> The commit you reference is actually older than the sctp gso patch, so
> it makes sense that it did not have a check in the sctp_gso_segment.
>
> I still think that we should check in inet_gso_segment when we have
> the proto, instead of in each {tcp, sctp, udp, esp, ...} handler having
> a check of the form.
>
>       !(type & (SKB_GSO_TCPV4 |
>         SKB_GSO_TCPV6))))

Unless we can create packets that legitimate combine
SKB_GSO_DODGY with tunnel headers.

virtio_net_hdr_to_skb does not accept tunneled gso types.

But a tun device can be bridged with a gre tunnel in the
host, creating a path that will call gre_gso_segment.

If that is possible, then this patch is indeed too strict and
we do need checks in the individual handlers.

Re: [PATCH net] gso: validate gso_type if SKB_GSO_DODGY

[Jason Wang]

On 2018年01月18日 14:04, Willem de Bruijn wrote:
> On Thu, Jan 18, 2018 at 12:20 AM, Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
>> On Wed, Jan 17, 2018 at 10:48 PM, Jason Wang <jasowang@redhat.com> wrote:
>>>
>>> On 2018年01月18日 07:11, Willem de Bruijn wrote:
>>>> From: Willem de Bruijn<willemb@google.com>
>>>>
>>>> Validate gso_type of untrusted SKB_GSO_DODGY packets during
>>>> segmentation.
>>>>
>>>> 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.
>>>>
>>>> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
>>>
>>> This commit is suspicious, I guess it should be 5c7cdf339af5 ("gso: Remove
>>> arbitrary checks for unsupported GSO")
>> The specific SCTP path was introduced with commit 90017accff61 ("sctp:
>> add GSO support"). But the main issue that packets can be delivered to
>> gso handlers different from their gso_type goes back further.
>>
>> The commit you reference is actually older than the sctp gso patch, so
>> it makes sense that it did not have a check in the sctp_gso_segment.
>>
>> I still think that we should check in inet_gso_segment when we have
>> the proto, instead of in each {tcp, sctp, udp, esp, ...} handler having
>> a check of the form.
>>
>>        !(type & (SKB_GSO_TCPV4 |
>>          SKB_GSO_TCPV6))))
> Unless we can create packets that legitimate combine
> SKB_GSO_DODGY with tunnel headers.

As you mentioned below, looks like we can e.g bridge between tunnels 
(vxlan, gre or others) and tap, or even bpf can produce this (e.g 
bpf_skb_adjust_room).

>
> virtio_net_hdr_to_skb does not accept tunneled gso types.

Yes, Vlad is trying to extend virtio to support more kinds of gso types, 
so it will be supported for sure.

>
> But a tun device can be bridged with a gre tunnel in the
> host, creating a path that will call gre_gso_segment.
>
> If that is possible, then this patch is indeed too strict and
> we do need checks in the individual handlers.

I think so.

Thanks

Re: [PATCH net] gso: validate gso_type if SKB_GSO_DODGY

[Willem de Bruijn]

On Thu, Jan 18, 2018 at 4:14 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2018年01月18日 14:04, Willem de Bruijn wrote:
>>
>> On Thu, Jan 18, 2018 at 12:20 AM, Willem de Bruijn
>> <willemdebruijn.kernel@gmail.com> wrote:
>>>
>>> On Wed, Jan 17, 2018 at 10:48 PM, Jason Wang <jasowang@redhat.com> wrote:
>>>>
>>>>
>>>> On 2018年01月18日 07:11, Willem de Bruijn wrote:
>>>>>
>>>>> From: Willem de Bruijn<willemb@google.com>
>>>>>
>>>>> Validate gso_type of untrusted SKB_GSO_DODGY packets during
>>>>> segmentation.
>>>>>
>>>>> 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.
>>>>>
>>>>> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
>>>>
>>>>
>>>> This commit is suspicious, I guess it should be 5c7cdf339af5 ("gso:
>>>> Remove
>>>> arbitrary checks for unsupported GSO")
>>>
>>> The specific SCTP path was introduced with commit 90017accff61 ("sctp:
>>> add GSO support"). But the main issue that packets can be delivered to
>>> gso handlers different from their gso_type goes back further.
>>>
>>> The commit you reference is actually older than the sctp gso patch, so
>>> it makes sense that it did not have a check in the sctp_gso_segment.
>>>
>>> I still think that we should check in inet_gso_segment when we have
>>> the proto, instead of in each {tcp, sctp, udp, esp, ...} handler having
>>> a check of the form.
>>>
>>>        !(type & (SKB_GSO_TCPV4 |
>>>          SKB_GSO_TCPV6))))
>>
>> Unless we can create packets that legitimate combine
>> SKB_GSO_DODGY with tunnel headers.
>
>
> As you mentioned below, looks like we can e.g bridge between tunnels (vxlan,
> gre or others) and tap, or even bpf can produce this (e.g
> bpf_skb_adjust_room).
>
>>
>> virtio_net_hdr_to_skb does not accept tunneled gso types.
>
>
> Yes, Vlad is trying to extend virtio to support more kinds of gso types, so
> it will be supported for sure.
>
>>
>> But a tun device can be bridged with a gre tunnel in the
>> host, creating a path that will call gre_gso_segment.
>>
>> If that is possible, then this patch is indeed too strict and
>> we do need checks in the individual handlers.

Okay, I'm working on a patch that adds explicit checks

  @@ -45,6 +45,9 @@ static struct sk_buff *sctp_gso_segment(struct sk_buff *skb,
          struct sk_buff *segs = ERR_PTR(-EINVAL);
          struct sctphdr *sh;

  +       if (!skb_shinfo(skb)->gso_type & SKB_GSO_SCTP)
  +               goto out;
  +

to all transport layer gso offloads: {sctp, tcpv[46], ufov[46], espv[46]}. This
will block packets with gso_type X but protocol type Y from being parsed.
But does allow entering a tunnel protocol handler if that is different from Y,
unlike the above patch.

tunnels segmentation itself is protected by skb->encapsulation. Only tunnel
devices in the stack can set this field, not virtio_net_hdr_to_skb. Packets that
enter {gre, udp tunnel, ipxip4, ipxip6} without this bit are already dropped, so
no new checks are needed to these based on gso_type.

That leaves eth_proto callbacks. mpls had this check before commit 5c7cdf339af5
("gso: Remove arbitrary checks for unsupported GSO"):

         if (unlikely(skb_shinfo(skb)->gso_type &
                                 ~(SKB_GSO_TCPV4 |
                                   SKB_GSO_TCPV6 |
                                   SKB_GSO_UDP |
                                   SKB_GSO_DODGY |
                                   SKB_GSO_TCP_FIXEDID |
                                   SKB_GSO_TCP_ECN)))
                 goto out;

which appears to exclude only other tunnel types at the time, no
transport layers. I don't think we need any new check here, then: it
was robust against handling dodgy sources.

Aside from inet_gso_segment and ipv6_gso_segment, this only leaves the
new nsh_gso_segment. Unlke mpls, it has its own gso_type, so

        if (!skb_shinfo(skb)->gso_type & SKB_GSO_NSH)
                goto out;

Re: [PATCH net] gso: validate gso_type if SKB_GSO_DODGY

[Willem de Bruijn]

> Aside from inet_gso_segment and ipv6_gso_segment, this only leaves the
> new nsh_gso_segment. Unlke mpls, it has its own gso_type, so
>
>         if (!skb_shinfo(skb)->gso_type & SKB_GSO_NSH)
>                 goto out;

This last point was incorrect. There is no such type for this protocol, so
we have to do the same as for MPLS. Either assume that it is robust
or be conservative and add

        if (skb_shinfo(skb)->gso_type & SKB_GSO_DODGY)
                goto out;

It looks correct on the surface and may already be relied upon, so I
suggest the first approach.

Re: [PATCH net] gso: validate gso_type if SKB_GSO_DODGY

[Willem de Bruijn]

> Okay, I'm working on a patch that adds explicit checks

Sent: http://patchwork.ozlabs.org/patch/863237/

>
>   @@ -45,6 +45,9 @@ static struct sk_buff *sctp_gso_segment(struct sk_buff *skb,
>           struct sk_buff *segs = ERR_PTR(-EINVAL);
>           struct sctphdr *sh;
>
>   +       if (!skb_shinfo(skb)->gso_type & SKB_GSO_SCTP)
>   +               goto out;
>   +
>
> to all transport layer gso offloads: {sctp, tcpv[46], ufov[46], espv[46]}. This
> will block packets with gso_type X but protocol type Y from being parsed.
> But does allow entering a tunnel protocol handler if that is different from Y,
> unlike the above patch.
>
> tunnels segmentation itself is protected by skb->encapsulation. Only tunnel
> devices in the stack can set this field, not virtio_net_hdr_to_skb. Packets that
> enter {gre, udp tunnel, ipxip4, ipxip6} without this bit are already dropped, so
> no new checks are needed to these based on gso_type.

This is not yet sufficient. If a packet comes from userspace with tunnel
headers and passes through a tunnel that sets skb->encapsulation, it is
indistinguishable from a valid tunneled packet. That bit is not exclusive,