From a2bebfd6e09d285aa793cae3fb0fc3a39a9fee6e Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Mon, 23 Mar 2015 22:58:21 +0000 Subject: CVE-2015-1779: incrementally decode websocket frames 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 [ kraxel: fix frequent spurious disconnects, suggested by Peter Maydell ] @@ -361,7 +361,7 @@ int vncws_decode_frame_payload(Buffer *input, - *payload_size = input->offset; + *payload_size = *payload_remain; [ kraxel: fix 32bit build ] @@ -306,7 +306,7 @@ struct VncState - uint64_t ws_payload_remain; + size_t ws_payload_remain; Signed-off-by: Gerd Hoffmann --- 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 85dbb7e6ae..0b7de4e662 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 ef229b7c0c..14d4230eff 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 e19ac396f2..3f7c6a9bc6 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; + size_t ws_payload_remain; + WsMask ws_payload_mask; #endif /* current output mode information */ VncWritePixels *write_pixels; -- cgit v1.2.3 From 2cdb5e142fb93e875fa53c52864ef5eb8d5d8b41 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Mon, 23 Mar 2015 22:58:22 +0000 Subject: CVE-2015-1779: limit size of HTTP headers from websockets clients 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 Signed-off-by: Gerd Hoffmann --- 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 0b7de4e662..62eb97fe76 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); } } -- cgit v1.2.3