On 2018年01月17日 22:27, Willem de Bruijn wrote:
On Wed, Jan 17, 2018 at 6:54 AM, Jason Wang <jasow...@redhat.com> wrote:
On 2018年01月17日 12:33, Willem de Bruijn wrote:
On Tue, Jan 16, 2018 at 11:04 PM, Jason Wang <jasow...@redhat.com> wrote:
On 2018年01月17日 04:29, Willem de Bruijn wrote:
From: Willem de Bruijn<will...@google.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
