CVE-2015-1779: fix denial of service in VNC websockets
cover CVE-2015-1779: fix denial of service in VNC websockets
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(-)
Signature · 1 line
2.1.0
PATCH 1/2 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.
--- ui/vnc-ws.c | 105 ++++++++++++++++++++++++++++++++++++++++-------------------- ui/vnc-ws.h | 9 ++++-- ui/vnc.h | 2 ++ 3 files changed, 80 insertions(+), 36 deletions(-)
ui/vnc-ws.c +71 -34
@@ -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 ;
ui/vnc-ws.h +7 -2
@@ -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 */
ui/vnc.h +2 -0
@@ -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;
Signature · 1 line
2.1.0
Re: [Qemu-devel] [PATCH 1/2] CVE-2015-1779: incrementally decode websocket frames
Quoted 8 lines · Daniel P. Berrange
ui/vnc-ws.c +6
-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) {
ui/vnc-ws.c int vncws_decode_frame(Buffer *input, uint8_t **payload, · line 358
Quoted 10 lines
ui/vnc-ws.c +9
-1-/* 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...
ui/vnc-ws.c int vncws_decode_frame(Buffer *input, uint8_t **payload, · line 363
Quoted 7 lines
ui/vnc-ws.c +3
-2+ } +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
Hi,
Quoted 22 lines
ui/vnc-ws.c int vncws_decode_frame(Buffer *input, uint8_t **payload, · line 358
Quoted 4 lines
ui/vnc-ws.c +4
-0+if (input->offset < *payload_remain) { + *payload_size = input->offset - (input->offset %4 ); + }else { + *payload_size = input->offset;This can set
*payload_sizeto a value larger than*payload_remain, if the input buffer happens to contain further data after the end of this packet...ui/vnc-ws.c int vncws_decode_frame(Buffer *input, uint8_t **payload, · line 363
Quoted 7 lines
ui/vnc-ws.c +3
-2+ } +if (*payload_size ==0 ) {return 0 ; } - - *payload = input->buffer + header_size; + *payload_remain -= *payload_size;...at which point this will end up making
*payload_remainnegative. Disconnection happens shortly afterwards.Should the line
Quoted 8 lines
*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
Quoted 8 lines · Gerd Hoffmann
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
Quoted 10 lines · Peter Maydell
Quoted 8 lines · Gerd Hoffmann
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 ...
Quoted 5 lines
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
Signature · 4 lines
|: 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 :|
Re: [Qemu-devel] [PATCH 1/2] CVE-2015-1779: incrementally decode websocket frames
In reply to Daniel P. BerrangeQuoted 13 lines · Daniel P. Berrange
ui/vnc-ws.c +2
-11-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;
ui/vnc-ws.c int vncws_decode_frame(Buffer *input, uint8_t **payload, · line 334
Quoted 10 lines
ui/vnc-ws.c +10
-0+ *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
PATCH 2/2 CVE-2015-1779: limit size of HTTP headers from websockets clients
In reply to Daniel P. BerrangeThe 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.
--- ui/vnc-ws.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
ui/vnc-ws.c +8 -2
@@ -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); } }
Signature · 1 line
2.1.0
Quoted 7 lines · 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.
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
Quoted 9 lines · Gerd Hoffmann
Quoted 7 lines · 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.
Quoted 3 lines
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