From e099d79879fa3662384d661a25593c661db4f823 Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Thu, 4 Apr 2019 14:58:15 +0200 Subject: [PATCH] Remove dangerous s_check() This macro checks if a pointer is valid _after_ we've already used that pointer. So it will only trigger if we're already performed some for of buffer overflow. As such, it provides little to no value and can only server to encourage broken behaviour. Let's remove it and replace it with proper bounds checking before access instead. --- asn.c | 14 +++++++++++++- lspci.c | 6 ------ mcs.c | 4 +--- orders.c | 12 +++--------- rdp.c | 3 +-- rdp5.c | 3 +-- rdpdr.c | 10 ---------- rdpsnd.c | 5 ----- seamless.c | 6 ------ secure.c | 10 +++++++++- stream.h | 3 +-- 11 files changed, 29 insertions(+), 47 deletions(-) diff --git a/asn.c b/asn.c index bd0577b..6c591c8 100644 --- a/asn.c +++ b/asn.c @@ -41,10 +41,16 @@ ber_parse_header(STREAM s, int tagval, uint32 *length) if (tagval > 0xff) { + if (!s_check_rem(s, 2)) { + return False; + } in_uint16_be(s, tag); } else { + if (!s_check_rem(s, 1)) { + return False; + } in_uint8(s, tag); } @@ -54,11 +60,17 @@ ber_parse_header(STREAM s, int tagval, uint32 *length) return False; } + if (!s_check_rem(s, 1)) { + return False; + } in_uint8(s, len); if (len & 0x80) { len &= ~0x80; + if (!s_check_rem(s, len)) { + return False; + } *length = 0; while (len--) next_be(s, *length); @@ -66,7 +78,7 @@ ber_parse_header(STREAM s, int tagval, uint32 *length) else *length = len; - return s_check(s); + return True; } void diff --git a/lspci.c b/lspci.c index 2d17161..7690a1f 100644 --- a/lspci.c +++ b/lspci.c @@ -135,12 +135,6 @@ 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 aeeb675..ed466a0 100644 --- a/mcs.c +++ b/mcs.c @@ -42,7 +42,7 @@ mcs_out_domain_params(STREAM s, int max_channels, int max_users, int max_tokens, } /* Parse a DOMAIN_PARAMS structure (ASN.1 BER) */ -static RD_BOOL +static void mcs_parse_domain_params(STREAM s) { uint32 length; @@ -56,8 +56,6 @@ mcs_parse_domain_params(STREAM s) } in_uint8s(s, length); - - return s_check(s); } /* Send an MCS_CONNECT_INITIAL message (ASN.1 BER) */ diff --git a/orders.c b/orders.c index bcc1a7e..44e5034 100644 --- a/orders.c +++ b/orders.c @@ -101,7 +101,7 @@ rdp_in_colour(STREAM s, uint32 * colour) } /* Parse bounds information */ -static RD_BOOL +static void rdp_parse_bounds(STREAM s, BOUNDS * bounds) { uint8 present; @@ -127,12 +127,10 @@ rdp_parse_bounds(STREAM s, BOUNDS * bounds) rdp_in_coord(s, &bounds->bottom, False); else if (present & 128) rdp_in_coord(s, &bounds->bottom, True); - - return s_check(s); } /* Parse a pen */ -static RD_BOOL +static void rdp_parse_pen(STREAM s, PEN * pen, uint32 present) { if (present & 1) @@ -143,8 +141,6 @@ rdp_parse_pen(STREAM s, PEN * pen, uint32 present) if (present & 4) rdp_in_colour(s, &pen->colour); - - return s_check(s); } static void @@ -176,7 +172,7 @@ setup_brush(BRUSH * out_brush, BRUSH * in_brush) } /* Parse a brush */ -static RD_BOOL +static void rdp_parse_brush(STREAM s, BRUSH * brush, uint32 present) { if (present & 1) @@ -193,8 +189,6 @@ rdp_parse_brush(STREAM s, BRUSH * brush, uint32 present) if (present & 16) in_uint8a(s, &brush->pattern[1], 7); - - return s_check(s); } /* Process a destination blt order */ diff --git a/rdp.c b/rdp.c index 0c9be79..2b12a01 100644 --- a/rdp.c +++ b/rdp.c @@ -1417,8 +1417,7 @@ process_pointer_pdu(STREAM s) case RDP_POINTER_MOVE: in_uint16_le(s, x); in_uint16_le(s, y); - if (s_check(s)) - ui_move_pointer(x, y); + ui_move_pointer(x, y); break; case RDP_POINTER_COLOR: diff --git a/rdp5.c b/rdp5.c index c5ef13b..5fce5f3 100644 --- a/rdp5.c +++ b/rdp5.c @@ -56,8 +56,7 @@ process_ts_fp_update_by_code(STREAM s, uint8 code) case FASTPATH_UPDATETYPE_PTR_POSITION: in_uint16_le(s, x); in_uint16_le(s, y); - if (s_check(s)) - ui_move_pointer(x, y); + ui_move_pointer(x, y); break; case FASTPATH_UPDATETYPE_COLOR: process_colour_pointer_pdu(s); diff --git a/rdpdr.c b/rdpdr.c index 402d94b..ac6ecff 100644 --- a/rdpdr.c +++ b/rdpdr.c @@ -877,7 +877,6 @@ 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); */ @@ -900,15 +899,6 @@ rdpdr_process(STREAM s) 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 73428c9..9289380 100644 --- a/rdpsnd.c +++ b/rdpsnd.c @@ -451,11 +451,6 @@ 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 1935074..0f294cc 100644 --- a/seamless.c +++ b/seamless.c @@ -376,12 +376,6 @@ 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 5dd686b..16755ff 100644 --- a/secure.c +++ b/secure.c @@ -523,6 +523,10 @@ sec_parse_public_key(STREAM s, uint8 * modulus, uint8 * exponent) { uint32 magic, modulus_len; + if (!s_check_rem(s, 8)) { + return False; + } + in_uint32_le(s, magic); if (magic != SEC_RSA_MAGIC) { @@ -541,13 +545,17 @@ sec_parse_public_key(STREAM s, uint8 * modulus, uint8 * exponent) return False; } + if (!s_check_rem(s, 1 + SEC_EXPONENT_SIZE + modulus_len + SEC_PADDING_SIZE)) { + return False; + } + in_uint8s(s, 8); /* modulus_bits, unknown */ in_uint8a(s, exponent, SEC_EXPONENT_SIZE); in_uint8a(s, modulus, modulus_len); in_uint8s(s, SEC_PADDING_SIZE); g_server_public_key_len = modulus_len; - return s_check(s); + return True; } /* Parse a public signature structure */ diff --git a/stream.h b/stream.h index f47dab7..5b2179f 100644 --- a/stream.h +++ b/stream.h @@ -55,8 +55,7 @@ size_t in_ansi_string(STREAM s, char *string, size_t len); #define s_push_layer(s,h,n) { (s)->h = (s)->p; (s)->p += n; } #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_check(s) && (n <= (s)->end - (s)->p)) +#define s_check_rem(s,n) (((s)->p <= (s)->end) && (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))