[Qemu-devel] [PULL for-2.3 0/2] vnc: fix websocket security issues (cve-2015-1779).

[Qemu-devel] [PULL for-2.3 0/2] vnc: fix websocket security issues (cve-2015-1779).

[Gerd Hoffmann]

  Hi,

$subject says all, here are the cve-2015-1779 fixes for vnc websockets
from Daniel P. Berrange for 2.3-rc2.

please pull,
  Gerd

The following changes since commit 054903a832b865eb5432d79b5c9d1e1ff31b58d7:

  Update version for v2.3.0-rc1 release (2015-03-24 16:34:16 +0000)

are available in the git repository at:

  git://git.kraxel.org/qemu tags/pull-cve-2015-1779-20150401-1

for you to fetch changes up to 9cf222fd4fd3f4d1f959685c061279d0673726cd:

  CVE-2015-1779: limit size of HTTP headers from websockets clients (2015-04-01 15:48:52 +0200)

----------------------------------------------------------------
vnc: fix websocket security issues (cve-2015-1779).

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

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

[Gerd Hoffmann]

From: "Daniel P. Berrange" <berrange@redhat.com>

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>

kraxel: Apply incremental fix suggested by Peter Maydell.

  @@ -361,7 +361,7 @@ int vncws_decode_frame_payload(Buffer *input,
  -        *payload_size = input->offset;
  +        *payload_size = *payload_remain;

Signed-off-by: Gerd Hoffmann <kraxel@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..0b7de4e 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 = *payload_remain;
+    }
+    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;
-- 
1.8.3.1

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

[Gerd Hoffmann]

From: "Daniel P. Berrange" <berrange@redhat.com>

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>
Signed-off-by: Gerd Hoffmann <kraxel@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 0b7de4e..62eb97f 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);
     }
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PULL for-2.3 0/2] vnc: fix websocket security issues (cve-2015-1779).

[Peter Maydell]

On 1 April 2015 at 15:11, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
> $subject says all, here are the cve-2015-1779 fixes for vnc websockets
> from Daniel P. Berrange for 2.3-rc2.
>
> please pull,
>   Gerd
>
> The following changes since commit 054903a832b865eb5432d79b5c9d1e1ff31b58d7:
>
>   Update version for v2.3.0-rc1 release (2015-03-24 16:34:16 +0000)
>
> are available in the git repository at:
>
>   git://git.kraxel.org/qemu tags/pull-cve-2015-1779-20150401-1
>
> for you to fetch changes up to 9cf222fd4fd3f4d1f959685c061279d0673726cd:
>
>   CVE-2015-1779: limit size of HTTP headers from websockets clients (2015-04-01 15:48:52 +0200)
>
> ----------------------------------------------------------------
> vnc: fix websocket security issues (cve-2015-1779).
>
> ----------------------------------------------------------------
> Daniel P. Berrange (2):
>       CVE-2015-1779: incrementally decode websocket frames
>       CVE-2015-1779: limit size of HTTP headers from websockets clients

Oops, this doesn't build on 32 bit:

/root/qemu/ui/vnc-ws.c: In function ‘vnc_client_read_ws’:
/root/qemu/ui/vnc-ws.c:133:45: error: passing argument 3 of
‘vncws_decode_frame_header’ from incompatible pointer type [-Werror]
In file included from /root/qemu/ui/vnc.h:112:0,
                 from /root/qemu/ui/vnc-ws.c:21:
/root/qemu/ui/vnc-ws.h:86:5: note: expected ‘size_t *’ but argument is
of type ‘uint64_t *’
/root/qemu/ui/vnc-ws.c:145:46: error: passing argument 2 of
‘vncws_decode_frame_payload’ from incompatible pointer type [-Werror]
In file included from /root/qemu/ui/vnc.h:112:0,
                 from /root/qemu/ui/vnc-ws.c:21:
/root/qemu/ui/vnc-ws.h:90:5: note: expected ‘size_t *’ but argument is
of type ‘uint64_t *’
cc1: all warnings being treated as errors

Making the payload_remain arguments to the two functions
be uint64_t rather than size_t seems to fix this.
The other approach is to change the VncState::ws_payload_remain
field from uint64_t to size_t. I haven't looked through the
ramifications of this enough to recommend one over the other.

Since we need to respin anyway, can you add the effects of
not having the fixes (ie "frequent spurious disconnects",
"fails to build on 32-bit systems") to the commit message
too?

thanks
-- PMM