[Qemu-devel] [PATCH 0/2] CVE-2015-1779: fix denial of service in VNC websockets

[Qemu-devel] [PATCH 0/2] CVE-2015-1779: fix denial of service in VNC websockets

[Daniel P. Berrange]

The VNC websockets protocol decoder has two places where it did
not correctly limit its resource usage when processing data from
the client. This can be abused by a malicious client to cause QEMU
to consume all system memory, unless it is otherwise limited by
ulimits and/or cgroups. These problems can be triggered in the
websockets layer before the VNC protocol actually starts, so no
client authentication will have taken place at this point.

Daniel P. Berrange (2):
  CVE-2015-1779: incrementally decode websocket frames
  CVE-2015-1779: limit size of HTTP headers from websockets clients

 ui/vnc-ws.c | 115 +++++++++++++++++++++++++++++++++++++++++-------------------
 ui/vnc-ws.h |   9 +++--
 ui/vnc.h    |   2 ++
 3 files changed, 88 insertions(+), 38 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/2] CVE-2015-1779: incrementally decode websocket frames

[Daniel P. Berrange]

The logic for decoding websocket frames wants to fully
decode the frame header and payload, before allowing the
VNC server to see any of the payload data. There is no
size limit on websocket payloads, so this allows a
malicious network client to consume 2^64 bytes in memory
in QEMU. It can trigger this denial of service before
the VNC server even performs any authentication.

The fix is to decode the header, and then incrementally
decode the payload data as it is needed. With this fix
the websocket decoder will allow at most 4k of data to
be buffered before decoding and processing payload.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 ui/vnc-ws.c | 105 ++++++++++++++++++++++++++++++++++++++++--------------------
 ui/vnc-ws.h |   9 ++++--
 ui/vnc.h    |   2 ++
 3 files changed, 80 insertions(+), 36 deletions(-)

diff --git a/ui/vnc-ws.c b/ui/vnc-ws.c
index 85dbb7e..e8146d0 100644
--- a/ui/vnc-ws.c
+++ b/ui/vnc-ws.c
@@ -107,7 +107,7 @@ long vnc_client_read_ws(VncState *vs)
 {
     int ret, err;
     uint8_t *payload;
-    size_t payload_size, frame_size;
+    size_t payload_size, header_size;
     VNC_DEBUG("Read websocket %p size %zd offset %zd\n", vs->ws_input.buffer,
             vs->ws_input.capacity, vs->ws_input.offset);
     buffer_reserve(&vs->ws_input, 4096);
@@ -117,18 +117,39 @@ long vnc_client_read_ws(VncState *vs)
     }
     vs->ws_input.offset += ret;
 
-    /* make sure that nothing is left in the ws_input buffer */
+    ret = 0;
+    /* consume as much of ws_input buffer as possible */
     do {
-        err = vncws_decode_frame(&vs->ws_input, &payload,
-                              &payload_size, &frame_size);
-        if (err <= 0) {
-            return err;
+        if (vs->ws_payload_remain == 0) {
+            err = vncws_decode_frame_header(&vs->ws_input,
+                                            &header_size,
+                                            &vs->ws_payload_remain,
+                                            &vs->ws_payload_mask);
+            if (err <= 0) {
+                return err;
+            }
+
+            buffer_advance(&vs->ws_input, header_size);
         }
+        if (vs->ws_payload_remain != 0) {
+            err = vncws_decode_frame_payload(&vs->ws_input,
+                                             &vs->ws_payload_remain,
+                                             &vs->ws_payload_mask,
+                                             &payload,
+                                             &payload_size);
+            if (err < 0) {
+                return err;
+            }
+            if (err == 0) {
+                return ret;
+            }
+            ret += err;
 
-        buffer_reserve(&vs->input, payload_size);
-        buffer_append(&vs->input, payload, payload_size);
+            buffer_reserve(&vs->input, payload_size);
+            buffer_append(&vs->input, payload, payload_size);
 
-        buffer_advance(&vs->ws_input, frame_size);
+            buffer_advance(&vs->ws_input, payload_size);
+        }
     } while (vs->ws_input.offset > 0);
 
     return ret;
@@ -265,15 +286,14 @@ void vncws_encode_frame(Buffer *output, const void *payload,
     buffer_append(output, payload, payload_size);
 }
 
-int vncws_decode_frame(Buffer *input, uint8_t **payload,
-                           size_t *payload_size, size_t *frame_size)
+int vncws_decode_frame_header(Buffer *input,
+                              size_t *header_size,
+                              size_t *payload_remain,
+                              WsMask *payload_mask)
 {
     unsigned char opcode = 0, fin = 0, has_mask = 0;
-    size_t header_size = 0;
-    uint32_t *payload32;
+    size_t payload_len;
     WsHeader *header = (WsHeader *)input->buffer;
-    WsMask mask;
-    int i;
 
     if (input->offset < WS_HEAD_MIN_LEN + 4) {
         /* header not complete */
@@ -283,7 +303,7 @@ int vncws_decode_frame(Buffer *input, uint8_t **payload,
     fin = (header->b0 & 0x80) >> 7;
     opcode = header->b0 & 0x0f;
     has_mask = (header->b1 & 0x80) >> 7;
-    *payload_size = header->b1 & 0x7f;
+    payload_len = header->b1 & 0x7f;
 
     if (opcode == WS_OPCODE_CLOSE) {
         /* disconnect */
@@ -300,40 +320,57 @@ int vncws_decode_frame(Buffer *input, uint8_t **payload,
         return -2;
     }
 
-    if (*payload_size < 126) {
-        header_size = 6;
-        mask = header->u.m;
-    } else if (*payload_size == 126 && input->offset >= 8) {
-        *payload_size = be16_to_cpu(header->u.s16.l16);
-        header_size = 8;
-        mask = header->u.s16.m16;
-    } else if (*payload_size == 127 && input->offset >= 14) {
-        *payload_size = be64_to_cpu(header->u.s64.l64);
-        header_size = 14;
-        mask = header->u.s64.m64;
+    if (payload_len < 126) {
+        *payload_remain = payload_len;
+        *header_size = 6;
+        *payload_mask = header->u.m;
+    } else if (payload_len == 126 && input->offset >= 8) {
+        *payload_remain = be16_to_cpu(header->u.s16.l16);
+        *header_size = 8;
+        *payload_mask = header->u.s16.m16;
+    } else if (payload_len == 127 && input->offset >= 14) {
+        *payload_remain = be64_to_cpu(header->u.s64.l64);
+        *header_size = 14;
+        *payload_mask = header->u.s64.m64;
     } else {
         /* header not complete */
         return 0;
     }
 
-    *frame_size = header_size + *payload_size;
+    return 1;
+}
+
+int vncws_decode_frame_payload(Buffer *input,
+                               size_t *payload_remain, WsMask *payload_mask,
+                               uint8_t **payload, size_t *payload_size)
+{
+    size_t i;
+    uint32_t *payload32;
 
-    if (input->offset < *frame_size) {
-        /* frame not complete */
+    *payload = input->buffer;
+    /* If we aren't at the end of the payload, then drop
+     * off the last bytes, so we're always multiple of 4
+     * for purpose of unmasking, except at end of payload
+     */
+    if (input->offset < *payload_remain) {
+        *payload_size = input->offset - (input->offset % 4);
+    } else {
+        *payload_size = input->offset;
+    }
+    if (*payload_size == 0) {
         return 0;
     }
-
-    *payload = input->buffer + header_size;
+    *payload_remain -= *payload_size;
 
     /* unmask frame */
     /* process 1 frame (32 bit op) */
     payload32 = (uint32_t *)(*payload);
     for (i = 0; i < *payload_size / 4; i++) {
-        payload32[i] ^= mask.u;
+        payload32[i] ^= payload_mask->u;
     }
     /* process the remaining bytes (if any) */
     for (i *= 4; i < *payload_size; i++) {
-        (*payload)[i] ^= mask.c[i % 4];
+        (*payload)[i] ^= payload_mask->c[i % 4];
     }
 
     return 1;
diff --git a/ui/vnc-ws.h b/ui/vnc-ws.h
index ef229b7..14d4230 100644
--- a/ui/vnc-ws.h
+++ b/ui/vnc-ws.h
@@ -83,7 +83,12 @@ long vnc_client_read_ws(VncState *vs);
 void vncws_process_handshake(VncState *vs, uint8_t *line, size_t size);
 void vncws_encode_frame(Buffer *output, const void *payload,
             const size_t payload_size);
-int vncws_decode_frame(Buffer *input, uint8_t **payload,
-                               size_t *payload_size, size_t *frame_size);
+int vncws_decode_frame_header(Buffer *input,
+                              size_t *header_size,
+                              size_t *payload_remain,
+                              WsMask *payload_mask);
+int vncws_decode_frame_payload(Buffer *input,
+                               size_t *payload_remain, WsMask *payload_mask,
+                               uint8_t **payload, size_t *payload_size);
 
 #endif /* __QEMU_UI_VNC_WS_H */
diff --git a/ui/vnc.h b/ui/vnc.h
index e19ac39..66af181 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -306,6 +306,8 @@ struct VncState
 #ifdef CONFIG_VNC_WS
     Buffer ws_input;
     Buffer ws_output;
+    uint64_t ws_payload_remain;
+    WsMask ws_payload_mask;
 #endif
     /* current output mode information */
     VncWritePixels *write_pixels;
-- 
2.1.0

[Qemu-devel] [PATCH 2/2] CVE-2015-1779: limit size of HTTP headers from websockets clients

[Daniel P. Berrange]

The VNC server websockets decoder will read and buffer data from
websockets clients until it sees the end of the HTTP headers,
as indicated by \r\n\r\n. In theory this allows a malicious to
trick QEMU into consuming an arbitrary amount of RAM. In practice,
because QEMU runs g_strstr_len() across the buffered header data,
it will spend increasingly long burning CPU time searching for
the substring match and less & less time reading data. So while
this does cause arbitrary memory growth, the bigger problem is
that QEMU will be burning 100% of available CPU time.

A novnc websockets client typically sends headers of around
512 bytes in length. As such it is reasonable to place a 4096
byte limit on the amount of data buffered while searching for
the end of HTTP headers.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 ui/vnc-ws.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/ui/vnc-ws.c b/ui/vnc-ws.c
index e8146d0..525e2d7 100644
--- a/ui/vnc-ws.c
+++ b/ui/vnc-ws.c
@@ -81,8 +81,11 @@ void vncws_handshake_read(void *opaque)
     VncState *vs = opaque;
     uint8_t *handshake_end;
     long ret;
-    buffer_reserve(&vs->ws_input, 4096);
-    ret = vnc_client_read_buf(vs, buffer_end(&vs->ws_input), 4096);
+    /* Typical HTTP headers from novnc are 512 bytes, so limiting
+     * total header size to 4096 is easily enough. */
+    size_t want = 4096 - vs->ws_input.offset;
+    buffer_reserve(&vs->ws_input, want);
+    ret = vnc_client_read_buf(vs, buffer_end(&vs->ws_input), want);
 
     if (!ret) {
         if (vs->csock == -1) {
@@ -99,6 +102,9 @@ void vncws_handshake_read(void *opaque)
         vncws_process_handshake(vs, vs->ws_input.buffer, vs->ws_input.offset);
         buffer_advance(&vs->ws_input, handshake_end - vs->ws_input.buffer +
                 strlen(WS_HANDSHAKE_END));
+    } else if (vs->ws_input.offset >= 4096) {
+        VNC_DEBUG("End of headers not found in first 4096 bytes\n");
+        vnc_client_error(vs);
     }
 }
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH 0/2] CVE-2015-1779: fix denial of service in VNC websockets

[Gerd Hoffmann]

On Mo, 2015-03-23 at 22:58 +0000, Daniel P. Berrange wrote:
> The VNC websockets protocol decoder has two places where it did
> not correctly limit its resource usage when processing data from
> the client. This can be abused by a malicious client to cause QEMU
> to consume all system memory, unless it is otherwise limited by
> ulimits and/or cgroups. These problems can be triggered in the
> websockets layer before the VNC protocol actually starts, so no
> client authentication will have taken place at this point.

Hmm, with patch 1/2 applied novnc disconnects frequently.  Boot messages
on the text (framebuffer) console seems to work fine.  But after logging
in via gdm and trying to do stuff in gnome shell problems are starting.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 0/2] CVE-2015-1779: fix denial of service in VNC websockets

[Peter Maydell]

On 24 March 2015 at 15:51, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Mo, 2015-03-23 at 22:58 +0000, Daniel P. Berrange wrote:
>> The VNC websockets protocol decoder has two places where it did
>> not correctly limit its resource usage when processing data from
>> the client. This can be abused by a malicious client to cause QEMU
>> to consume all system memory, unless it is otherwise limited by
>> ulimits and/or cgroups. These problems can be triggered in the
>> websockets layer before the VNC protocol actually starts, so no
>> client authentication will have taken place at this point.
>
> Hmm, with patch 1/2 applied novnc disconnects frequently.  Boot messages
> on the text (framebuffer) console seems to work fine.  But after logging
> in via gdm and trying to do stuff in gnome shell problems are starting.

Any further progress here? Ideally I'd like to get a fix for
this CVE into rc2...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] CVE-2015-1779: incrementally decode websocket frames

[Peter Maydell]

On 23 March 2015 at 22:58, Daniel P. Berrange <berrange@redhat.com> wrote:
> +int vncws_decode_frame_payload(Buffer *input,
> +                               size_t *payload_remain, WsMask *payload_mask,
> +                               uint8_t **payload, size_t *payload_size)
> +{
> +    size_t i;
> +    uint32_t *payload32;
>
> -    if (input->offset < *frame_size) {
> -        /* frame not complete */
> +    *payload = input->buffer;
> +    /* If we aren't at the end of the payload, then drop
> +     * off the last bytes, so we're always multiple of 4
> +     * for purpose of unmasking, except at end of payload
> +     */
> +    if (input->offset < *payload_remain) {
> +        *payload_size = input->offset - (input->offset % 4);
> +    } else {
> +        *payload_size = input->offset;

This can set *payload_size to a value larger than
*payload_remain, if the input buffer happens to contain
further data after the end of this packet...

> +    }
> +    if (*payload_size == 0) {
>          return 0;
>      }
> -
> -    *payload = input->buffer + header_size;
> +    *payload_remain -= *payload_size;

...at which point this will end up making
*payload_remain negative. Disconnection happens shortly
afterwards.

Should the line
    *payload_size = input->offset;
actually read
    *payload_size = *payload_remain;

?

Making that change appears to fix the novnc disconnects
that Gerd reports.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] CVE-2015-1779: incrementally decode websocket frames

[Peter Maydell]

On 23 March 2015 at 22:58, Daniel P. Berrange <berrange@redhat.com> wrote:
> -    if (*payload_size < 126) {
> -        header_size = 6;
> -        mask = header->u.m;
> -    } else if (*payload_size == 126 && input->offset >= 8) {
> -        *payload_size = be16_to_cpu(header->u.s16.l16);
> -        header_size = 8;
> -        mask = header->u.s16.m16;
> -    } else if (*payload_size == 127 && input->offset >= 14) {
> -        *payload_size = be64_to_cpu(header->u.s64.l64);
> -        header_size = 14;
> -        mask = header->u.s64.m64;
> +    if (payload_len < 126) {
> +        *payload_remain = payload_len;
> +        *header_size = 6;
> +        *payload_mask = header->u.m;
> +    } else if (payload_len == 126 && input->offset >= 8) {
> +        *payload_remain = be16_to_cpu(header->u.s16.l16);
> +        *header_size = 8;
> +        *payload_mask = header->u.s16.m16;
> +    } else if (payload_len == 127 && input->offset >= 14) {
> +        *payload_remain = be64_to_cpu(header->u.s64.l64);
> +        *header_size = 14;
> +        *payload_mask = header->u.s64.m64;

We were already doing this before, but if this is a 32 bit
machine then the assignment to *payload_remain in this
case is going to be assigning a 64-bit value from the datastream
to a 32-bit size_t, which doesn't seem like a great idea
to just silently do, though I suppose the datastream is in
complete control of that value anyway.

-- PMM

Re: [Qemu-devel] [PATCH 1/2] CVE-2015-1779: incrementally decode websocket frames

[Gerd Hoffmann]

  Hi,

> > +    if (input->offset < *payload_remain) {
> > +        *payload_size = input->offset - (input->offset % 4);
> > +    } else {
> > +        *payload_size = input->offset;
> 
> This can set *payload_size to a value larger than
> *payload_remain, if the input buffer happens to contain
> further data after the end of this packet...
> 
> > +    }
> > +    if (*payload_size == 0) {
> >          return 0;
> >      }
> > -
> > -    *payload = input->buffer + header_size;
> > +    *payload_remain -= *payload_size;
> 
> ...at which point this will end up making
> *payload_remain negative. Disconnection happens shortly
> afterwards.
> 
> Should the line
>     *payload_size = input->offset;
> actually read
>     *payload_size = *payload_remain;
> 
> ?
> 
> Making that change appears to fix the novnc disconnects
> that Gerd reports.

Confirmed.  Fixes the issues I've seen in testing and looks sensible to
me.  Comment from Daniel would be nice, especially as I know next to
nothing about websockets, but he seems to be off into the easter
holidays already.

So, with -rc2 waiting for this (and being late already) I think I'll
squash in the incremental fix and prepare a pull request even without
Daniels ack ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] CVE-2015-1779: incrementally decode websocket frames

[Peter Maydell]

On 1 April 2015 at 14:36, Gerd Hoffmann <kraxel@redhat.com> wrote:
> Confirmed.  Fixes the issues I've seen in testing and looks sensible to
> me.  Comment from Daniel would be nice, especially as I know next to
> nothing about websockets, but he seems to be off into the easter
> holidays already.
>
> So, with -rc2 waiting for this (and being late already) I think I'll
> squash in the incremental fix and prepare a pull request even without
> Daniels ack ...

Yes, that seems best. Given that this is a CVE fix can you
make sure the change is called out clearly in the commit
message so it's easy for downstreams to see which version
of the fix they have applied? Might be worth including the
fixup-diff in the commit message...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] CVE-2015-1779: incrementally decode websocket frames

[Daniel P. Berrange]

On Wed, Apr 01, 2015 at 02:41:57PM +0100, Peter Maydell wrote:
> On 1 April 2015 at 14:36, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > Confirmed.  Fixes the issues I've seen in testing and looks sensible to
> > me.  Comment from Daniel would be nice, especially as I know next to
> > nothing about websockets, but he seems to be off into the easter
> > holidays already.
> >
> > So, with -rc2 waiting for this (and being late already) I think I'll
> > squash in the incremental fix and prepare a pull request even without
> > Daniels ack ...
> 
> Yes, that seems best. Given that this is a CVE fix can you
> make sure the change is called out clearly in the commit
> message so it's easy for downstreams to see which version
> of the fix they have applied? Might be worth including the
> fixup-diff in the commit message...

Yes, that fix looks correct to me too, thanks for figuring that out.

Sorry for not responding before - I've been off on paternity leave
for several weeks and only just catching up.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|