From 4dca546d04321a610c1835010b5dad85163b65e1 Mon Sep 17 00:00:00 2001 From: Henrik Andersson Date: Wed, 16 Jan 2019 10:45:33 +0100 Subject: [PATCH] Malicious RDP server security fixes This commit includes fixes for a set of 21 vulnerabilities in rdesktop when a malicious RDP server is used. All vulnerabilities was identified and reported by Eyal Itkin. * Add rdp_protocol_error function that is used in several fixes * Refactor of process_bitmap_updates * Fix possible integer overflow in s_check_rem() on 32bit arch * Fix memory corruption in process_bitmap_data - CVE-2018-8794 * Fix remote code execution in process_bitmap_data - CVE-2018-8795 * Fix remote code execution in process_plane - CVE-2018-8797 * Fix Denial of Service in mcs_recv_connect_response - CVE-2018-20175 * Fix Denial of Service in mcs_parse_domain_params - CVE-2018-20175 * Fix Denial of Service in sec_parse_crypt_info - CVE-2018-20176 * Fix Denial of Service in sec_recv - CVE-2018-20176 * Fix minor information leak in rdpdr_process - CVE-2018-8791 * Fix Denial of Service in cssp_read_tsrequest - CVE-2018-8792 * Fix remote code execution in cssp_read_tsrequest - CVE-2018-8793 * Fix Denial of Service in process_bitmap_data - CVE-2018-8796 * Fix minor information leak in rdpsnd_process_ping - CVE-2018-8798 * Fix Denial of Service in process_secondary_order - CVE-2018-8799 * Fix remote code execution in in ui_clip_handle_data - CVE-2018-8800 * Fix major information leak in ui_clip_handle_data - CVE-2018-20174 * Fix memory corruption in rdp_in_unistr - CVE-2018-20177 * Fix Denial of Service in process_demand_active - CVE-2018-20178 * Fix remote code execution in lspci_process - CVE-2018-20179 * Fix remote code execution in rdpsnddbg_process - CVE-2018-20180 * Fix remote code execution in seamless_process - CVE-2018-20181 * Fix remote code execution in seamless_process_line - CVE-2018-20182 --- asn.c | 2 +- bitmap.c | 8 +-- cliprdr.c | 6 ++ constants.h | 3 + cssp.c | 17 ++++- lspci.c | 9 ++- mcs.c | 20 +++++- orders.c | 6 ++ proto.h | 3 +- rdp.c | 202 +++++++++++++++++++++++++++++++++++----------------- rdpdr.c | 11 +++ rdpsnd.c | 11 +++ seamless.c | 12 ++++ secure.c | 17 +++++ stream.h | 2 +- types.h | 2 + 16 files changed, 255 insertions(+), 76 deletions(-) diff --git a/asn.c b/asn.c index 26b0e9b..1b9fea4 100644 --- a/asn.c +++ b/asn.c @@ -22,7 +22,7 @@ /* Parse an ASN.1 BER header */ RD_BOOL -ber_parse_header(STREAM s, int tagval, int *length) +ber_parse_header(STREAM s, int tagval, uint32 *length) { int tag, len; diff --git a/bitmap.c b/bitmap.c index 775622c..55c768c 100644 --- a/bitmap.c +++ b/bitmap.c @@ -794,7 +794,7 @@ process_plane(uint8 * in, int width, int height, uint8 * out, int size) replen = revcode; collen = 0; } - while (collen > 0) + while (indexw < width && collen > 0) { color = CVAL(in); *out = color; @@ -802,7 +802,7 @@ process_plane(uint8 * in, int width, int height, uint8 * out, int size) indexw++; collen--; } - while (replen > 0) + while (indexw < width && replen > 0) { *out = color; out += 4; @@ -824,7 +824,7 @@ process_plane(uint8 * in, int width, int height, uint8 * out, int size) replen = revcode; collen = 0; } - while (collen > 0) + while (indexw < width && collen > 0) { x = CVAL(in); if (x & 1) @@ -844,7 +844,7 @@ process_plane(uint8 * in, int width, int height, uint8 * out, int size) indexw++; collen--; } - while (replen > 0) + while (indexw < width && replen > 0) { x = last_line[indexw * 4] + color; *out = x; diff --git a/cliprdr.c b/cliprdr.c index 3f68213..76eb98d 100644 --- a/cliprdr.c +++ b/cliprdr.c @@ -118,6 +118,7 @@ cliprdr_process(STREAM s) uint16 type, status; uint32 length, format; uint8 *data; + struct stream packet = *s; in_uint16_le(s, type); in_uint16_le(s, status); @@ -127,6 +128,11 @@ cliprdr_process(STREAM s) logger(Clipboard, Debug, "cliprdr_process(), type=%d, status=%d, length=%d", type, status, length); + if (!s_check_rem(s, length)) + { + rdp_protocol_error("cliprdr_process(), consume of packet from stream would overrun", &packet); + } + if (status == CLIPRDR_ERROR) { switch (type) diff --git a/constants.h b/constants.h index 7dfc2e3..6301502 100644 --- a/constants.h +++ b/constants.h @@ -751,6 +751,9 @@ enum RDP_DESKTOP_ORIENTATION #define ENC_SALTED_CHECKSUM 0x0010 #define NO_BITMAP_COMPRESSION_HDR 0x0400 +/* [MS-RDPBCGR], TS_BITMAP_DATA, flags */ +#define BITMAP_COMPRESSION 0x0001 + /* orderFlags, [MS-RDPBCGR] 2.2.7.1.3 */ #define NEGOTIATEORDERSUPPORT 0x0002 #define ZEROBOUNDSDELTASSUPPORT 0x0008 diff --git a/cssp.c b/cssp.c index 33544c3..6b2d0f7 100644 --- a/cssp.c +++ b/cssp.c @@ -595,6 +595,7 @@ cssp_read_tsrequest(STREAM token, STREAM pubkey) STREAM s; int length; int tagval; + struct stream packet; s = tcp_recv(NULL, 4); @@ -622,6 +623,7 @@ cssp_read_tsrequest(STREAM token, STREAM pubkey) // receive the remainings of message s = tcp_recv(s, length); + packet = *s; // parse the response and into nego token if (!ber_in_header(s, &tagval, &length) || @@ -632,6 +634,12 @@ cssp_read_tsrequest(STREAM token, STREAM pubkey) if (!ber_in_header(s, &tagval, &length) || tagval != (BER_TAG_CTXT_SPECIFIC | BER_TAG_CONSTRUCTED | 0)) return False; + + if (!s_check_rem(s, length)) + { + rdp_protocol_error("cssp_read_tsrequest(), consume of version from stream would overrun", + &packet); + } in_uint8s(s, length); // negoToken [1] @@ -653,7 +661,14 @@ cssp_read_tsrequest(STREAM token, STREAM pubkey) if (!ber_in_header(s, &tagval, &length) || tagval != BER_TAG_OCTET_STRING) return False; - token->end = token->p = token->data; + if (!s_check_rem(s, length)) + { + rdp_protocol_error("cssp_read_tsrequest(), consume of token from stream would overrun", + &packet); + } + + s_realloc(token, length); + s_reset(token); out_uint8p(token, s->p, length); s_mark_end(token); } diff --git a/lspci.c b/lspci.c index c4c8ce6..2d17161 100644 --- a/lspci.c +++ b/lspci.c @@ -1,7 +1,8 @@ /* -*- c-basic-offset: 8 -*- rdesktop: A Remote Desktop Protocol client. Support for the Matrox "lspci" channel - Copyright (C) 2005 Matrox Graphics Inc. + Copyright (C) 2005 Matrox Graphics Inc. + Copyright 2018 Henrik Andersson for Cendio AB This program is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -134,6 +135,12 @@ lspci_process(STREAM s) unsigned int pkglen; static char *rest = NULL; char *buf; + struct stream packet = *s; + + if (!s_check(s)) + { + rdp_protocol_error("lspci_process(), stream is in unstable state", &packet); + } pkglen = s->end - s->p; /* str_handle_lines requires null terminated strings */ diff --git a/mcs.c b/mcs.c index 10b73a1..0f2360e 100644 --- a/mcs.c +++ b/mcs.c @@ -45,9 +45,16 @@ mcs_out_domain_params(STREAM s, int max_channels, int max_users, int max_tokens, static RD_BOOL mcs_parse_domain_params(STREAM s) { - int length; + uint32 length; + struct stream packet = *s; ber_parse_header(s, MCS_TAG_DOMAIN_PARAMS, &length); + + if (!s_check_rem(s, length)) + { + rdp_protocol_error("mcs_parse_domain_params(), consume domain params from stream would overrun", &packet); + } + in_uint8s(s, length); return s_check(s); @@ -89,8 +96,9 @@ mcs_recv_connect_response(STREAM mcs_data) { UNUSED(mcs_data); uint8 result; - int length; + uint32 length; STREAM s; + struct stream packet; RD_BOOL is_fastpath; uint8 fastpath_hdr; @@ -99,6 +107,8 @@ mcs_recv_connect_response(STREAM mcs_data) if (s == NULL) return False; + + packet = *s; ber_parse_header(s, MCS_CONNECT_RESPONSE, &length); @@ -112,6 +122,12 @@ mcs_recv_connect_response(STREAM mcs_data) ber_parse_header(s, BER_TAG_INTEGER, &length); in_uint8s(s, length); /* connect id */ + + if (!s_check_rem(s, length)) + { + rdp_protocol_error("mcs_recv_connect_response(), consume connect id from stream would overrun", &packet); + } + mcs_parse_domain_params(s); ber_parse_header(s, BER_TAG_OCTET_STRING, &length); diff --git a/orders.c b/orders.c index ba9cfa1..5e34770 100644 --- a/orders.c +++ b/orders.c @@ -1259,11 +1259,17 @@ process_secondary_order(STREAM s) uint16 flags; uint8 type; uint8 *next_order; + struct stream packet = *s; in_uint16_le(s, length); in_uint16_le(s, flags); /* used by bmpcache2 */ in_uint8(s, type); + if (!s_check_rem(s, length + 7)) + { + rdp_protocol_error("process_secondary_order(), next order pointer would overrun stream", &packet); + } + next_order = s->p + (sint16) length + 7; switch (type) diff --git a/proto.h b/proto.h index 73449dd..0d6a6c9 100644 --- a/proto.h +++ b/proto.h @@ -164,6 +164,7 @@ RD_BOOL rdp_connect(char *server, uint32 flags, char *domain, char *password, ch char *directory, RD_BOOL reconnect); void rdp_reset_state(void); void rdp_disconnect(void); +void rdp_protocol_error(const char *message, STREAM s); /* rdpdr.c */ int get_device_index(RD_NTHANDLE handle); void convert_to_unix_filename(char *filename); @@ -224,7 +225,7 @@ void tcp_run_ui(RD_BOOL run); /* asn.c */ RD_BOOL ber_in_header(STREAM s, int *tagval, int *length); void ber_out_header(STREAM s, int tagval, int length); -RD_BOOL ber_parse_header(STREAM s, int tagval, int *length); +RD_BOOL ber_parse_header(STREAM s, int tagval, uint32 *length); void ber_out_integer(STREAM s, int value); void ber_out_sequence(STREAM s, STREAM contents); diff --git a/rdp.c b/rdp.c index f98f49c..1654e58 100644 --- a/rdp.c +++ b/rdp.c @@ -285,6 +285,19 @@ rdp_in_unistr(STREAM s, int in_len, char **string, uint32 * str_size) size_t ibl, obl; char *pin, *pout; + struct stream packet = *s; + + if ((in_len < 0) || ((uint32)in_len >= (RD_UINT32_MAX / 2))) + { + logger(Protocol, Error, "rdp_in_unistr(), length of unicode data is out of bounds."); + abort(); + } + + if (!s_check_rem(s, in_len)) + { + rdp_protocol_error("rdp_in_unistr(), consume of unicode data from stream would overrun", &packet); + } + // if not already open if (!icv_utf16_to_local) { @@ -1211,6 +1224,7 @@ process_demand_active(STREAM s) { uint8 type; uint16 len_src_descriptor, len_combined_caps; + struct stream packet = *s; /* at this point we need to ensure that we have ui created */ rd_create_ui(); @@ -1218,6 +1232,11 @@ process_demand_active(STREAM s) in_uint32_le(s, g_rdp_shareid); in_uint16_le(s, len_src_descriptor); in_uint16_le(s, len_combined_caps); + + if (!s_check_rem(s, len_src_descriptor)) + { + rdp_protocol_error("rdp_demand_active(), consume of source descriptor from stream would overrun", &packet); + } in_uint8s(s, len_src_descriptor); logger(Protocol, Debug, "process_demand_active(), shareid=0x%x", g_rdp_shareid); @@ -1390,78 +1409,113 @@ process_pointer_pdu(STREAM s) } } -/* Process bitmap updates */ +/* Process TS_BITMAP_DATA */ +static void +process_bitmap_data(STREAM s) +{ + uint16 left, top, right, bottom, width, height; + uint16 cx, cy, bpp, Bpp, flags, bufsize, size; + uint8 *data, *bmpdata; + + logger(Protocol, Debug, "%s()", __func__); + + struct stream packet = *s; + + in_uint16_le(s, left); /* destLeft */ + in_uint16_le(s, top); /* destTop */ + in_uint16_le(s, right); /* destRight */ + in_uint16_le(s, bottom); /* destBottom */ + in_uint16_le(s, width); /* width */ + in_uint16_le(s, height); /* height */ + in_uint16_le(s, bpp); /*bitsPerPixel */ + Bpp = (bpp + 7) / 8; + in_uint16_le(s, flags); /* flags */ + in_uint16_le(s, bufsize); /* bitmapLength */ + + cx = right - left + 1; + cy = bottom - top + 1; + + /* FIXME: There are a assumtion that we do not consider in + this code. The value of bpp is not passed to + ui_paint_bitmap() which relies on g_server_bpp for drawing + the bitmap data. + + Does this means that we can sanity check bpp with g_server_bpp ? + */ + + if (Bpp == 0 || width == 0 || height == 0) + { + logger(Protocol, Warning, "%s(), [%d,%d,%d,%d], [%d,%d], bpp=%d, flags=%x", __func__, + left, top, right, bottom, width, height, bpp, flags); + rdp_protocol_error("TS_BITMAP_DATA, unsafe size of bitmap data received from server", &packet); + } + + if ((RD_UINT32_MAX / Bpp) <= (width * height)) + { + logger(Protocol, Warning, "%s(), [%d,%d,%d,%d], [%d,%d], bpp=%d, flags=%x", __func__, + left, top, right, bottom, width, height, bpp, flags); + rdp_protocol_error("TS_BITMAP_DATA, unsafe size of bitmap data received from server", &packet); + } + + if (flags == 0) + { + /* read uncompressed bitmap data */ + int y; + bmpdata = (uint8 *) xmalloc(width * height * Bpp); + for (y = 0; y < height; y++) + { + in_uint8a(s, &bmpdata[(height - y - 1) * (width * Bpp)], width * Bpp); + } + + ui_paint_bitmap(left, top, cx, cy, width, height, bmpdata); + xfree(bmpdata); + return; + } + + if (flags & NO_BITMAP_COMPRESSION_HDR) + { + size = bufsize; + } + else + { + /* Read TS_CD_HEADER */ + in_uint8s(s, 2); /* skip cbCompFirstRowSize (must be 0x0000) */ + in_uint16_le(s, size); /* cbCompMainBodySize */ + in_uint8s(s, 2); /* skip cbScanWidth */ + in_uint8s(s, 2); /* skip cbUncompressedSize */ + } + + /* read compressed bitmap data */ + if (!s_check_rem(s, size)) + { + rdp_protocol_error("process_bitmap_data(), consume of bitmap data from stream would overrun", &packet); + } + in_uint8p(s, data, size); + bmpdata = (uint8 *) xmalloc(width * height * Bpp); + if (bitmap_decompress(bmpdata, width, height, data, size, Bpp)) + { + ui_paint_bitmap(left, top, cx, cy, width, height, bmpdata); + } + else + { + logger(Protocol, Warning, "%s(), failed to decompress bitmap", __func__); + } + + xfree(bmpdata); +} + +/* Process TS_UPDATE_BITMAP_DATA */ void process_bitmap_updates(STREAM s) { - uint16 num_updates; - uint16 left, top, right, bottom, width, height; - uint16 cx, cy, bpp, Bpp, compress, bufsize, size; - uint8 *data, *bmpdata; int i; - - logger(Protocol, Debug, "%s()", __func__); - - in_uint16_le(s, num_updates); + uint16 num_updates; + + in_uint16_le(s, num_updates); /* rectangles */ for (i = 0; i < num_updates; i++) { - in_uint16_le(s, left); - in_uint16_le(s, top); - in_uint16_le(s, right); - in_uint16_le(s, bottom); - in_uint16_le(s, width); - in_uint16_le(s, height); - in_uint16_le(s, bpp); - Bpp = (bpp + 7) / 8; - in_uint16_le(s, compress); - in_uint16_le(s, bufsize); - - cx = right - left + 1; - cy = bottom - top + 1; - - logger(Graphics, Debug, - "process_bitmap_updates(), [%d,%d,%d,%d], [%d,%d], bpp=%d, compression=%d", - left, top, right, bottom, width, height, Bpp, compress); - - if (!compress) - { - int y; - bmpdata = (uint8 *) xmalloc(width * height * Bpp); - for (y = 0; y < height; y++) - { - in_uint8a(s, &bmpdata[(height - y - 1) * (width * Bpp)], - width * Bpp); - } - ui_paint_bitmap(left, top, cx, cy, width, height, bmpdata); - xfree(bmpdata); - continue; - } - - - if (compress & 0x400) - { - size = bufsize; - } - else - { - in_uint8s(s, 2); /* pad */ - in_uint16_le(s, size); - in_uint8s(s, 4); /* line_size, final_size */ - } - in_uint8p(s, data, size); - bmpdata = (uint8 *) xmalloc(width * height * Bpp); - if (bitmap_decompress(bmpdata, width, height, data, size, Bpp)) - { - ui_paint_bitmap(left, top, cx, cy, width, height, bmpdata); - } - else - { - logger(Graphics, Warning, - "process_bitmap_updates(), failed to decompress bitmap"); - } - - xfree(bmpdata); + process_bitmap_data(s); } } @@ -2013,3 +2067,21 @@ rdp_disconnect(void) logger(Protocol, Debug, "%s()", __func__); sec_disconnect(); } + +/* Abort rdesktop upon protocol error + + A protocol error is defined as: + + - A value is outside specified range for example; + bpp for a bitmap is not allowed to be greater than the + value 32 but is represented by a byte in protocol. + +*/ +void +rdp_protocol_error(const char *message, STREAM s) +{ + logger(Protocol, Error, "%s(), %s", __func__, message); + if (s) + hexdump(s->p, s_length(s)); + exit(0); +} diff --git a/rdpdr.c b/rdpdr.c index e8f5431..0abcacd 100644 --- a/rdpdr.c +++ b/rdpdr.c @@ -854,6 +854,7 @@ rdpdr_process(STREAM s) uint16 vmin; uint16 component; uint16 pakid; + struct stream packet = *s; logger(Protocol, Debug, "rdpdr_process()"); /* hexdump(s->p, s->end - s->p); */ @@ -873,8 +874,18 @@ rdpdr_process(STREAM s) /* DR_CORE_SERVER_ANNOUNCE_REQ */ in_uint8s(s, 2); /* skip versionMajor */ in_uint16_le(s, vmin); /* VersionMinor */ + in_uint32_le(s, g_client_id); /* ClientID */ + /* g_client_id is sent back to server, + so lets check that we actually got + valid data from stream to prevent + that we leak back data to server */ + if (!s_check(s)) + { + rdp_protocol_error("rdpdr_process(), consume of g_client_id from stream did overrun", &packet); + } + /* The RDP client is responsibility to provide a random client id if server version is < 12 */ if (vmin < 0x000c) diff --git a/rdpsnd.c b/rdpsnd.c index 71a3bf2..40881b7 100644 --- a/rdpsnd.c +++ b/rdpsnd.c @@ -269,6 +269,12 @@ rdpsnd_process_training(STREAM in) uint16 tick; uint16 packsize; STREAM out; + struct stream packet = *in; + + if (!s_check_rem(in, 4)) + { + rdp_protocol_error("rdpsnd_process_training(), consume of training data from stream would overrun", &packet); + } in_uint16_le(in, tick); in_uint16_le(in, packsize); @@ -445,6 +451,11 @@ rdpsnddbg_process(STREAM s) static char *rest = NULL; char *buf; + if (!s_check(s)) + { + rdp_protocol_error("rdpsnddbg_process(), stream is in unstable state", s); + } + pkglen = s->end - s->p; /* str_handle_lines requires null terminated strings */ buf = (char *) xmalloc(pkglen + 1); diff --git a/seamless.c b/seamless.c index 8fde8dd..1935074 100644 --- a/seamless.c +++ b/seamless.c @@ -168,6 +168,12 @@ seamless_process_line(const char *line, void *data) icon_buf[len] = strtol(byte, NULL, 16); len++; + + if ((size_t)len >= sizeof(icon_buf)) + { + logger(Protocol, Warning, "seamless_process_line(), icon data would overrun icon_buf"); + break; + } } ui_seamless_seticon(id, tok5, width, height, chunk, icon_buf, len); @@ -370,6 +376,12 @@ seamless_process(STREAM s) { unsigned int pkglen; char *buf; + struct stream packet = *s; + + if (!s_check(s)) + { + rdp_protocol_error("seamless_process(), stream is in unstable state", &packet); + } pkglen = s->end - s->p; /* str_handle_lines requires null terminated strings */ diff --git a/secure.c b/secure.c index 48d9d01..f548a4f 100644 --- a/secure.c +++ b/secure.c @@ -296,6 +296,9 @@ sec_encrypt(uint8 * data, int length) void sec_decrypt(uint8 * data, int length) { + if (length <= 0) + return; + if (g_sec_decrypt_use_count == 4096) { sec_update(g_sec_decrypt_key, g_sec_decrypt_update_key); @@ -848,9 +851,11 @@ sec_recv(RD_BOOL * is_fastpath) uint16 sec_flags; uint16 channel; STREAM s; + struct stream packet; while ((s = mcs_recv(&channel, is_fastpath, &fastpath_hdr)) != NULL) { + packet = *s; if (*is_fastpath == True) { /* If fastpath packet is encrypted, read data @@ -859,6 +864,10 @@ sec_recv(RD_BOOL * is_fastpath) fastpath_flags = (fastpath_hdr & 0xC0) >> 6; if (fastpath_flags & FASTPATH_OUTPUT_ENCRYPTED) { + if (!s_check_rem(s, 8)) { + rdp_protocol_error("sec_recv(), consume fastpath signature from stream would overrun", &packet); + } + in_uint8s(s, 8); /* signature */ sec_decrypt(s->p, s->end - s->p); } @@ -875,6 +884,10 @@ sec_recv(RD_BOOL * is_fastpath) { if (sec_flags & SEC_ENCRYPT) { + if (!s_check_rem(s, 8)) { + rdp_protocol_error("sec_recv(), consume encrypt signature from stream would overrun", &packet); + } + in_uint8s(s, 8); /* signature */ sec_decrypt(s->p, s->end - s->p); } @@ -889,6 +902,10 @@ sec_recv(RD_BOOL * is_fastpath) { uint8 swapbyte; + if (!s_check_rem(s, 8)) { + rdp_protocol_error("sec_recv(), consume redirect signature from stream would overrun", &packet); + } + in_uint8s(s, 8); /* signature */ sec_decrypt(s->p, s->end - s->p); diff --git a/stream.h b/stream.h index ccc125d..a9196e5 100644 --- a/stream.h +++ b/stream.h @@ -54,7 +54,7 @@ size_t in_ansi_string(STREAM s, char *string, size_t len); #define s_pop_layer(s,h) (s)->p = (s)->h; #define s_mark_end(s) (s)->end = (s)->p; #define s_check(s) ((s)->p <= (s)->end) -#define s_check_rem(s,n) ((s)->p + n <= (s)->end) +#define s_check_rem(s,n) (s_check(s) && (n <= (s)->end - (s)->p)) #define s_check_end(s) ((s)->p == (s)->end) #define s_length(s) ((s)->end - (s)->data) #define s_left(s) ((s)->size - ((s)->p - (s)->data)) diff --git a/types.h b/types.h index b139b86..4556ab4 100644 --- a/types.h +++ b/types.h @@ -43,6 +43,8 @@ typedef signed short sint16; typedef unsigned int uint32; typedef signed int sint32; +#define RD_UINT32_MAX (uint32)(-1) + typedef void *RD_HBITMAP; typedef void *RD_HGLYPH; typedef void *RD_HCOLOURMAP;