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.
This commit is contained in:
Pierre Ossman
parent
de59a100ea
commit
e099d79879
14
asn.c
14
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
|
||||
|
||||
6
lspci.c
6
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 */
|
||||
|
||||
4
mcs.c
4
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) */
|
||||
|
||||
12
orders.c
12
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 */
|
||||
|
||||
3
rdp.c
3
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:
|
||||
|
||||
3
rdp5.c
3
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);
|
||||
|
||||
10
rdpdr.c
10
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)
|
||||
|
||||
5
rdpsnd.c
5
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);
|
||||
|
||||
@ -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 */
|
||||
|
||||
10
secure.c
10
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 */
|
||||
|
||||
3
stream.h
3
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))
|
||||
|
||||
Loading…
Reference in New Issue
Block a user