From 7ba714ad7fe8256edfaad7d9a0f09aeb9611ca44 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Thu, 30 Jan 2003 04:20:44 +0000 Subject: [PATCH] 2003-01-30 Havoc Pennington * dbus/dbus-message.c: use message->byte_order instead of DBUS_COMPILER_BYTE_ORDER throughout. (dbus_message_create_header): pad header to align the start of the body of the message to 8-byte boundary * dbus/dbus-marshal.h: make all the demarshalers take const DBusString arguments. * dbus/dbus-message.c (_dbus_message_loader_return_buffer): validate message args here, so we don't have to do slow validation later, and so we catch bad messages as they are incoming. Also add better checks on header_len and body_len. Also fill in message->byte_order * dbus/dbus-string.c (_dbus_string_validate_utf8): new (not implemented properly) (_dbus_string_validate_nul): new function to check all-nul * dbus/dbus-marshal.c (_dbus_marshal_get_field_end_pos): rename get_arg_end_pos and remove all validation (_dbus_marshal_validate_arg): actually do validation here. --- ChangeLog | 24 ++ dbus/dbus-marshal.c | 371 +++++++++++++++++++---- dbus/dbus-marshal.h | 99 +++--- dbus/dbus-message.c | 191 +++++++++--- dbus/dbus-string.c | 61 +++- dbus/dbus-string.h | 8 + test/data/valid-messages/opposite-endian.message | 3 +- test/data/valid-messages/simplest-manual.message | 1 + test/data/valid-messages/simplest.message | 1 + 9 files changed, 615 insertions(+), 144 deletions(-) diff --git a/ChangeLog b/ChangeLog index 7e0f9cd..2fced37 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,27 @@ +2003-01-30 Havoc Pennington + + * dbus/dbus-message.c: use message->byte_order instead of + DBUS_COMPILER_BYTE_ORDER throughout. + (dbus_message_create_header): pad header to align the + start of the body of the message to 8-byte boundary + + * dbus/dbus-marshal.h: make all the demarshalers take const + DBusString arguments. + + * dbus/dbus-message.c (_dbus_message_loader_return_buffer): + validate message args here, so we don't have to do slow validation + later, and so we catch bad messages as they are incoming. Also add + better checks on header_len and body_len. Also fill in + message->byte_order + + * dbus/dbus-string.c (_dbus_string_validate_utf8): new (not + implemented properly) + (_dbus_string_validate_nul): new function to check all-nul + + * dbus/dbus-marshal.c (_dbus_marshal_get_field_end_pos): rename + get_arg_end_pos and remove all validation + (_dbus_marshal_validate_arg): actually do validation here. + 2003-01-29 Havoc Pennington * dbus/dbus-message.c (check_message_handling): fix assertion diff --git a/dbus/dbus-marshal.c b/dbus/dbus-marshal.c index 9c17aab..efdc7ef 100644 --- a/dbus/dbus-marshal.c +++ b/dbus/dbus-marshal.c @@ -73,7 +73,7 @@ _dbus_unpack_uint32 (int byte_order, return DBUS_UINT32_FROM_LE (*(dbus_uint32_t*)data); else return DBUS_UINT32_FROM_BE (*(dbus_uint32_t*)data); -} +} /** * Unpacks a 32 bit signed integer from a data pointer @@ -504,7 +504,7 @@ _dbus_marshal_string_array (DBusString *str, * @returns the demarshaled double. */ double -_dbus_demarshal_double (DBusString *str, +_dbus_demarshal_double (const DBusString *str, int byte_order, int pos, int *new_pos) @@ -537,7 +537,7 @@ _dbus_demarshal_double (DBusString *str, * @returns the demarshaled integer. */ dbus_int32_t -_dbus_demarshal_int32 (DBusString *str, +_dbus_demarshal_int32 (const DBusString *str, int byte_order, int pos, int *new_pos) @@ -564,7 +564,7 @@ _dbus_demarshal_int32 (DBusString *str, * @returns the demarshaled integer. */ dbus_uint32_t -_dbus_demarshal_uint32 (DBusString *str, +_dbus_demarshal_uint32 (const DBusString *str, int byte_order, int pos, int *new_pos) @@ -598,7 +598,7 @@ _dbus_demarshal_uint32 (DBusString *str, * @returns the demarshaled string. */ char * -_dbus_demarshal_string (DBusString *str, +_dbus_demarshal_string (const DBusString *str, int byte_order, int pos, int *new_pos) @@ -606,7 +606,7 @@ _dbus_demarshal_string (DBusString *str, int len; char *retval; const char *data; - + len = _dbus_demarshal_uint32 (str, byte_order, pos, &pos); retval = dbus_malloc (len + 1); @@ -641,7 +641,7 @@ _dbus_demarshal_string (DBusString *str, * @returns the demarshaled data. */ unsigned char * -_dbus_demarshal_byte_array (DBusString *str, +_dbus_demarshal_byte_array (const DBusString *str, int byte_order, int pos, int *new_pos, @@ -685,7 +685,7 @@ _dbus_demarshal_byte_array (DBusString *str, * @returns the demarshaled data. */ dbus_int32_t * -_dbus_demarshal_int32_array (DBusString *str, +_dbus_demarshal_int32_array (const DBusString *str, int byte_order, int pos, int *new_pos, @@ -724,7 +724,7 @@ _dbus_demarshal_int32_array (DBusString *str, * @returns the demarshaled data. */ dbus_uint32_t * -_dbus_demarshal_uint32_array (DBusString *str, +_dbus_demarshal_uint32_array (const DBusString *str, int byte_order, int pos, int *new_pos, @@ -763,7 +763,7 @@ _dbus_demarshal_uint32_array (DBusString *str, * @returns the demarshaled data. */ double * -_dbus_demarshal_double_array (DBusString *str, +_dbus_demarshal_double_array (const DBusString *str, int byte_order, int pos, int *new_pos, @@ -802,7 +802,7 @@ _dbus_demarshal_double_array (DBusString *str, * @returns the demarshaled data. */ char ** -_dbus_demarshal_string_array (DBusString *str, +_dbus_demarshal_string_array (const DBusString *str, int byte_order, int pos, int *new_pos, @@ -843,32 +843,24 @@ _dbus_demarshal_string_array (DBusString *str, } /** - * Returns the position right after the end position - * end position of a field. Validates the field - * contents as required (e.g. ensures that - * string fields have a valid length and - * are nul-terminated). + * Returns the position right after the end of an argument. PERFORMS + * NO VALIDATION WHATSOEVER. The message must have been previously + * validated. * - * @todo security: audit the field validation code. - * - * @todo warns on invalid type in a message, but - * probably the whole message needs to be dumped, - * or we might even drop the connection due - * to bad protocol. Needs better error handling. - * Possible security issue. + * @todo handle DBUS_TYPE_NIL * * @param str a string * @param byte_order the byte order to use - * @param pos the pos where the field starts + * @param pos the pos where the arg starts * @param end_pos pointer where the position right * after the end position will follow - * @returns TRUE if more data exists after the field + * @returns TRUE if more data exists after the arg */ dbus_bool_t -_dbus_marshal_get_field_end_pos (DBusString *str, - int byte_order, - int pos, - int *end_pos) +_dbus_marshal_get_arg_end_pos (const DBusString *str, + int byte_order, + int pos, + int *end_pos) { const char *data; @@ -906,21 +898,8 @@ _dbus_marshal_get_field_end_pos (DBusString *str, len = _dbus_demarshal_uint32 (str, byte_order, pos + 1, &pos); *end_pos = pos + len + 1; - - if (*end_pos > _dbus_string_get_length (str)) - { - _dbus_verbose ("string length outside length of the message\n"); - return FALSE; - } - - if (_dbus_string_get_byte (str, pos+len) != '\0') - { - _dbus_verbose ("string field not nul-terminated\n"); - return FALSE; - } - - break; } + break; case DBUS_TYPE_BYTE_ARRAY: { @@ -930,9 +909,8 @@ _dbus_marshal_get_field_end_pos (DBusString *str, len = _dbus_demarshal_uint32 (str, byte_order, pos + 1, &pos); *end_pos = pos + len; - - break; } + break; case DBUS_TYPE_INT32_ARRAY: { @@ -943,9 +921,8 @@ _dbus_marshal_get_field_end_pos (DBusString *str, *end_pos = _DBUS_ALIGN_VALUE (new_pos, sizeof (dbus_int32_t)) + (len * sizeof (dbus_int32_t)); - - break; } + break; case DBUS_TYPE_UINT32_ARRAY: { @@ -956,9 +933,8 @@ _dbus_marshal_get_field_end_pos (DBusString *str, *end_pos = _DBUS_ALIGN_VALUE (new_pos, sizeof (dbus_uint32_t)) + (len * sizeof (dbus_uint32_t)); - - break; } + break; case DBUS_TYPE_DOUBLE_ARRAY: { @@ -969,9 +945,8 @@ _dbus_marshal_get_field_end_pos (DBusString *str, *end_pos = _DBUS_ALIGN_VALUE (new_pos, sizeof (double)) + (len * sizeof (double)); - - break; } + break; case DBUS_TYPE_STRING_ARRAY: { @@ -990,10 +965,262 @@ _dbus_marshal_get_field_end_pos (DBusString *str, } *end_pos = pos; - break; - } + } + break; + + default: + _dbus_warn ("Unknown message arg type %d\n", *data); + _dbus_assert_not_reached ("Unknown message argument type\n"); + return FALSE; + } + + if (*end_pos > _dbus_string_get_length (str)) + return FALSE; + + return TRUE; +} + +/** + * Demarshals and validates a length; returns < 0 if the validation + * fails. The length is required to be small enough that + * len*sizeof(double) will not overflow, and small enough to fit in a + * signed integer. + * + * @param str the string + * @param byte_order the byte order + * @param pos the unaligned string position (snap to next aligned) + */ +static int +demarshal_and_validate_len (const DBusString *str, + int byte_order, + int pos, + int *new_pos) +{ + int align_4 = _DBUS_ALIGN_VALUE (pos, 4); + unsigned int len; + + if ((align_4 + 4) >= _dbus_string_get_length (str)) + { + _dbus_verbose ("not enough room in message for array length\n"); + return -1; + } + + if (!_dbus_string_validate_nul (str, pos, + align_4 - pos)) + { + _dbus_verbose ("array length alignment padding not initialized to nul\n"); + return -1; + } + + len = _dbus_demarshal_uint32 (str, byte_order, align_4, new_pos); + + /* note that the len may be a number of doubles, so we need it to be + * at least SIZE_T_MAX / 8, but make it smaller just to keep things + * sane. We end up using ints for most sizes to avoid unsigned mess + * so limit to maximum 32-bit signed int divided by at least 8, more + * for a bit of paranoia margin. INT_MAX/32 is about 65 megabytes. + */ +#define MAX_ARRAY_LENGTH (((unsigned int)_DBUS_INT_MAX) / 32) + if (len > MAX_ARRAY_LENGTH) + { + _dbus_verbose ("array length %u exceeds maximum of %u\n", + len, MAX_ARRAY_LENGTH); + return -1; + } + else + return (int) len; +} + +static dbus_bool_t +validate_string (const DBusString *str, + int pos, + int len_without_nul, + int *end_pos) +{ + *end_pos = pos + len_without_nul + 1; + + if (*end_pos > _dbus_string_get_length (str)) + { + _dbus_verbose ("string length outside length of the message\n"); + return FALSE; + } + + if (_dbus_string_get_byte (str, pos + len_without_nul) != '\0') + { + _dbus_verbose ("string arg not nul-terminated\n"); + return FALSE; + } + + if (!_dbus_string_validate_utf8 (str, pos, len_without_nul)) + { + _dbus_verbose ("string is not valid UTF-8\n"); + return FALSE; + } + + return TRUE; +} + +/** + * Validates an argument, checking that it is well-formed, for example + * no ludicrous length fields, strings are nul-terminated, etc. + * Returns the end position of the argument in end_pos, and + * returns #TRUE if a valid arg begins at "pos" + * + * @todo security: need to audit this function. + * + * @todo handle DBUS_TYPE_NIL + * + * @param str a string + * @param byte_order the byte order to use + * @param pos the pos where the arg starts (offset of its typecode) + * @param end_pos pointer where the position right + * after the end position will follow + * @returns #TRUE if the arg is valid. + */ +dbus_bool_t +_dbus_marshal_validate_arg (const DBusString *str, + int byte_order, + int pos, + int *end_pos) +{ + const char *data; + + if (pos >= _dbus_string_get_length (str)) + return FALSE; + + _dbus_string_get_const_data_len (str, &data, pos, 1); + + switch (*data) + { + case DBUS_TYPE_INVALID: + return FALSE; + break; + + case DBUS_TYPE_INT32: + case DBUS_TYPE_UINT32: + { + int align_4 = _DBUS_ALIGN_VALUE (pos + 1, 4); + + if (!_dbus_string_validate_nul (str, pos + 1, + align_4 - pos - 1)) + { + _dbus_verbose ("int32/uint32 alignment padding not initialized to nul\n"); + return FALSE; + } + + *end_pos = align_4 + 4; + } + break; + + case DBUS_TYPE_DOUBLE: + { + int align_8 = _DBUS_ALIGN_VALUE (pos + 1, 8); + + _dbus_verbose_bytes_of_string (str, pos, (align_8 + 8 - pos)); + + if (!_dbus_string_validate_nul (str, pos + 1, + align_8 - pos - 1)) + { + _dbus_verbose ("double alignment padding not initialized to nul\n"); + return FALSE; + } + + *end_pos = align_8 + 8; + } + break; + + case DBUS_TYPE_STRING: + { + int len; + + /* Demarshal the length, which does NOT include + * nul termination + */ + len = demarshal_and_validate_len (str, byte_order, pos + 1, &pos); + if (len < 0) + return FALSE; + + if (!validate_string (str, pos, len, end_pos)) + return FALSE; + } + break; + + case DBUS_TYPE_BYTE_ARRAY: + { + int len; + + len = demarshal_and_validate_len (str, byte_order, pos + 1, &pos); + if (len < 0) + return FALSE; + + *end_pos = pos + len; + } + break; + + case DBUS_TYPE_INT32_ARRAY: + case DBUS_TYPE_UINT32_ARRAY: + { + int len; + + len = demarshal_and_validate_len (str, byte_order, pos + 1, &pos); + if (len < 0) + return FALSE; + + _dbus_assert (_DBUS_ALIGN_VALUE (pos, 4) == (unsigned int) pos); + + *end_pos = pos + len * 4; + } + break; + + case DBUS_TYPE_DOUBLE_ARRAY: + { + int len; + int align_8; + + len = demarshal_and_validate_len (str, byte_order, pos + 1, &pos); + if (len < 0) + return FALSE; + + align_8 = _DBUS_ALIGN_VALUE (pos, 8); + if (!_dbus_string_validate_nul (str, pos, + align_8 - pos)) + { + _dbus_verbose ("double array alignment padding not initialized to nul\n"); + return FALSE; + } + + *end_pos = align_8 + len * 8; + } + break; + + case DBUS_TYPE_STRING_ARRAY: + { + int len; + int i; + + len = demarshal_and_validate_len (str, byte_order, pos + 1, &pos); + if (len < 0) + return FALSE; + + for (i = 0; i < len; i++) + { + int str_len; + + str_len = demarshal_and_validate_len (str, byte_order, + pos, &pos); + if (str_len < 0) + return FALSE; + + if (!validate_string (str, pos, str_len, &pos)) + return FALSE; + } + + *end_pos = pos; + } + break; + default: - _dbus_warn ("Unknown message field type %d\n", *data); + _dbus_warn ("Unknown message arg type %d\n", *data); return FALSE; } @@ -1003,6 +1230,7 @@ _dbus_marshal_get_field_end_pos (DBusString *str, return TRUE; } + /** * If in verbose mode, print a block of binary data. * @@ -1018,6 +1246,8 @@ _dbus_verbose_bytes (const unsigned char *data, int i; const unsigned char *aligned; + _dbus_assert (len >= 0); + /* Print blanks on first row if appropriate */ aligned = _DBUS_ALIGN_ADDRESS (data, 4); if (aligned > data) @@ -1026,7 +1256,7 @@ _dbus_verbose_bytes (const unsigned char *data, if (aligned != data) { - _dbus_verbose ("%5d\t%p: ", - (data - aligned), aligned); + _dbus_verbose ("%4d\t%p: ", - (data - aligned), aligned); while (aligned != data) { _dbus_verbose (" "); @@ -1040,7 +1270,7 @@ _dbus_verbose_bytes (const unsigned char *data, { if (_DBUS_ALIGN_ADDRESS (&data[i], 4) == &data[i]) { - _dbus_verbose ("%5d\t%p: ", + _dbus_verbose ("%4d\t%p: ", i, &data[i]); } @@ -1056,9 +1286,16 @@ _dbus_verbose_bytes (const unsigned char *data, if (_DBUS_ALIGN_ADDRESS (&data[i], 4) == &data[i]) { if (i > 3) - _dbus_verbose ("big: %d little: %d", + _dbus_verbose ("BE: %d LE: %d", _dbus_unpack_uint32 (DBUS_BIG_ENDIAN, &data[i-4]), _dbus_unpack_uint32 (DBUS_LITTLE_ENDIAN, &data[i-4])); + + if (i > 7 && + _DBUS_ALIGN_ADDRESS (&data[i], 8) == &data[i]) + { + _dbus_verbose (" dbl: %g", + *(double*)&data[i-8]); + } _dbus_verbose ("\n"); } @@ -1080,7 +1317,27 @@ _dbus_verbose_bytes_of_string (const DBusString *str, int len) { const char *d; + int real_len; + + real_len = _dbus_string_get_length (str); + _dbus_assert (start >= 0); + + if (start > real_len) + { + _dbus_verbose (" [%d,%d) is not inside string of length %d\n", + start, len, real_len); + return; + } + + if ((start + len) > real_len) + { + _dbus_verbose (" [%d,%d) extends outside string of length %d\n", + start, len, real_len); + len = real_len - start; + } + + _dbus_string_get_const_data_len (str, &d, start, len); _dbus_verbose_bytes (d, len); diff --git a/dbus/dbus-marshal.h b/dbus/dbus-marshal.h index bdeeccb..cedca6b 100644 --- a/dbus/dbus-marshal.h +++ b/dbus/dbus-marshal.h @@ -124,54 +124,57 @@ dbus_bool_t _dbus_marshal_string_array (DBusString *str, const char **value, int len); -double _dbus_demarshal_double (DBusString *str, - int byte_order, - int pos, - int *new_pos); -dbus_int32_t _dbus_demarshal_int32 (DBusString *str, - int byte_order, - int pos, - int *new_pos); -dbus_uint32_t _dbus_demarshal_uint32 (DBusString *str, - int byte_order, - int pos, - int *new_pos); -char * _dbus_demarshal_string (DBusString *str, - int byte_order, - int pos, - int *new_pos); -unsigned char *_dbus_demarshal_byte_array (DBusString *str, - int byte_order, - int pos, - int *new_pos, - int *array_len); -dbus_int32_t *_dbus_demarshal_int32_array (DBusString *str, - int byte_order, - int pos, - int *new_pos, - int *array_len); -dbus_uint32_t *_dbus_demarshal_uint32_array (DBusString *str, - int byte_order, - int pos, - int *new_pos, - int *array_len); -double *_dbus_demarshal_double_array (DBusString *str, - int byte_order, - int pos, - int *new_pos, - int *array_len); -char **_dbus_demarshal_string_array (DBusString *str, - int byte_order, - int pos, - int *new_pos, - int *array_len); - -dbus_bool_t _dbus_marshal_get_field_end_pos (DBusString *str, - int byte_order, - int pos, - int *end_pos); - - +double _dbus_demarshal_double (const DBusString *str, + int byte_order, + int pos, + int *new_pos); +dbus_int32_t _dbus_demarshal_int32 (const DBusString *str, + int byte_order, + int pos, + int *new_pos); +dbus_uint32_t _dbus_demarshal_uint32 (const DBusString *str, + int byte_order, + int pos, + int *new_pos); +char * _dbus_demarshal_string (const DBusString *str, + int byte_order, + int pos, + int *new_pos); +unsigned char *_dbus_demarshal_byte_array (const DBusString *str, + int byte_order, + int pos, + int *new_pos, + int *array_len); +dbus_int32_t * _dbus_demarshal_int32_array (const DBusString *str, + int byte_order, + int pos, + int *new_pos, + int *array_len); +dbus_uint32_t *_dbus_demarshal_uint32_array (const DBusString *str, + int byte_order, + int pos, + int *new_pos, + int *array_len); +double * _dbus_demarshal_double_array (const DBusString *str, + int byte_order, + int pos, + int *new_pos, + int *array_len); +char ** _dbus_demarshal_string_array (const DBusString *str, + int byte_order, + int pos, + int *new_pos, + int *array_len); + + +dbus_bool_t _dbus_marshal_get_arg_end_pos (const DBusString *str, + int byte_order, + int pos, + int *end_pos); +dbus_bool_t _dbus_marshal_validate_arg (const DBusString *str, + int byte_order, + int pos, + int *end_pos); #endif /* DBUS_PROTOCOL_H */ diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c index b1dd856..85b49cb 100644 --- a/dbus/dbus-message.c +++ b/dbus/dbus-message.c @@ -91,6 +91,7 @@ struct DBusMessage HeaderField header_fields[FIELD_LAST]; /**< Track the location * of each field in "header" */ + int header_padding; /**< bytes of alignment in header */ DBusString body; /**< Body network data. */ @@ -138,6 +139,27 @@ _dbus_message_get_network_data (DBusMessage *message, } static void +clear_header_padding (DBusMessage *message) +{ + _dbus_string_shorten (&message->header, + message->header_padding); + message->header_padding = 0; +} + +static dbus_bool_t +append_header_padding (DBusMessage *message) +{ + int old_len; + old_len = _dbus_string_get_length (&message->header); + if (!_dbus_string_align_length (&message->header, 8)) + return FALSE; + + message->header_padding = _dbus_string_get_length (&message->header) - old_len; + + return TRUE; +} + +static void adjust_field_offsets (DBusMessage *message, int offsets_after, int delta) @@ -209,6 +231,8 @@ append_int_field (DBusMessage *message, int orig_len; _dbus_assert (!message->locked); + + clear_header_padding (message); orig_len = _dbus_string_get_length (&message->header); @@ -227,15 +251,24 @@ append_int_field (DBusMessage *message, message->header_fields[FIELD_REPLY_SERIAL].offset = _dbus_string_get_length (&message->header); - if (!_dbus_marshal_int32 (&message->header, DBUS_COMPILER_BYTE_ORDER, + if (!_dbus_marshal_int32 (&message->header, message->byte_order, value)) goto failed; + if (!append_header_padding (message)) + goto failed; + return TRUE; failed: message->header_fields[field].offset = -1; _dbus_string_set_length (&message->header, orig_len); + + /* this must succeed because it was allocated on function entry and + * DBusString doesn't ever realloc smaller + */ + if (!append_header_padding (message)) + _dbus_assert_not_reached ("failed to reappend header padding"); return FALSE; } @@ -248,6 +281,8 @@ append_string_field (DBusMessage *message, int orig_len; _dbus_assert (!message->locked); + + clear_header_padding (message); orig_len = _dbus_string_get_length (&message->header); @@ -266,15 +301,25 @@ append_string_field (DBusMessage *message, message->header_fields[field].offset = _dbus_string_get_length (&message->header); - if (!_dbus_marshal_string (&message->header, DBUS_COMPILER_BYTE_ORDER, + if (!_dbus_marshal_string (&message->header, message->byte_order, value)) goto failed; + if (!append_header_padding (message)) + goto failed; + return TRUE; failed: message->header_fields[field].offset = -1; _dbus_string_set_length (&message->header, orig_len); + + /* this must succeed because it was allocated on function entry and + * DBusString doesn't ever realloc smaller + */ + if (!append_header_padding (message)) + _dbus_assert_not_reached ("failed to reappend header padding"); + return FALSE; } @@ -290,6 +335,8 @@ delete_int_field (DBusMessage *message, if (offset < 0) return; + clear_header_padding (message); + /* The field typecode and name take up 8 bytes */ _dbus_string_delete (&message->header, offset - 8, @@ -300,6 +347,8 @@ delete_int_field (DBusMessage *message, adjust_field_offsets (message, offset - 8, - 12); + + append_header_padding (message); } static void @@ -316,6 +365,8 @@ delete_string_field (DBusMessage *message, if (offset < 0) return; + clear_header_padding (message); + get_string_field (message, field, &len); /* The field typecode and name take up 8 bytes, and the nul @@ -332,6 +383,8 @@ delete_string_field (DBusMessage *message, adjust_field_offsets (message, offset - 8, - delete_len); + + append_header_padding (message); } static dbus_bool_t @@ -528,8 +581,8 @@ static dbus_bool_t dbus_message_create_header (DBusMessage *message, const char *service, const char *name) -{ - if (!_dbus_string_append_byte (&message->header, DBUS_COMPILER_BYTE_ORDER)) +{ + if (!_dbus_string_append_byte (&message->header, message->byte_order)) return FALSE; if (!_dbus_string_append_len (&message->header, "\0\0\0", 3)) @@ -563,7 +616,7 @@ dbus_message_create_header (DBusMessage *message, DBUS_HEADER_FIELD_NAME, name)) return FALSE; - + return TRUE; } @@ -969,7 +1022,7 @@ dbus_message_append_int32 (DBusMessage *message, } return _dbus_marshal_int32 (&message->body, - DBUS_COMPILER_BYTE_ORDER, value); + message->byte_order, value); } /** @@ -992,7 +1045,7 @@ dbus_message_append_uint32 (DBusMessage *message, } return _dbus_marshal_uint32 (&message->body, - DBUS_COMPILER_BYTE_ORDER, value); + message->byte_order, value); } /** @@ -1015,7 +1068,7 @@ dbus_message_append_double (DBusMessage *message, } return _dbus_marshal_double (&message->body, - DBUS_COMPILER_BYTE_ORDER, value); + message->byte_order, value); } /** @@ -1038,7 +1091,7 @@ dbus_message_append_string (DBusMessage *message, } return _dbus_marshal_string (&message->body, - DBUS_COMPILER_BYTE_ORDER, value); + message->byte_order, value); } /** @@ -1063,7 +1116,7 @@ dbus_message_append_byte_array (DBusMessage *message, } return _dbus_marshal_byte_array (&message->body, - DBUS_COMPILER_BYTE_ORDER, value, len); + message->byte_order, value, len); } /** @@ -1088,7 +1141,7 @@ dbus_message_append_string_array (DBusMessage *message, } return _dbus_marshal_string_array (&message->body, - DBUS_COMPILER_BYTE_ORDER, value, len); + message->byte_order, value, len); } /** @@ -1342,9 +1395,9 @@ dbus_message_iter_has_next (DBusMessageIter *iter) { int end_pos; - if (!_dbus_marshal_get_field_end_pos (&iter->message->body, - iter->message->byte_order, - iter->pos, &end_pos)) + if (!_dbus_marshal_get_arg_end_pos (&iter->message->body, + iter->message->byte_order, + iter->pos, &end_pos)) return FALSE; if (end_pos >= _dbus_string_get_length (&iter->message->body)) @@ -1364,8 +1417,9 @@ dbus_message_iter_next (DBusMessageIter *iter) { int end_pos; - if (!_dbus_marshal_get_field_end_pos (&iter->message->body, iter->message->byte_order, - iter->pos, &end_pos)) + if (!_dbus_marshal_get_arg_end_pos (&iter->message->body, + iter->message->byte_order, + iter->pos, &end_pos)) return FALSE; if (end_pos >= _dbus_string_get_length (&iter->message->body)) @@ -1570,6 +1624,12 @@ dbus_message_get_sender (DBusMessage *message) * */ +/* we definitely use signed ints for sizes, so don't exceed + * _DBUS_INT_MAX; and add 16 for paranoia, since a message + * over 128M is pretty nuts anyhow. + */ +#define MAX_SANE_MESSAGE_SIZE (_DBUS_INT_MAX/16) + /** * Implementation details of DBusMessageLoader. * All members are private. @@ -1724,18 +1784,11 @@ _dbus_message_loader_get_buffer (DBusMessageLoader *loader, #define DBUS_HEADER_FIELD_SENDER_AS_UINT32 \ FOUR_CHARS_TO_UINT32 ('s', 'n', 'd', 'r') - -/* FIXME should be using DBusString for the stuff we demarshal. char* - * evil. Also, out of memory handling here seems suboptimal. - * Should probably report it as a distinct error from "corrupt message," - * so we can postpone parsing this message. Also, we aren't - * checking for demarshal failure everywhere. - */ static dbus_bool_t -decode_header_data (DBusString *data, - int header_len, - int byte_order, - HeaderField fields[FIELD_LAST]) +decode_header_data (const DBusString *data, + int header_len, + int byte_order, + HeaderField fields[FIELD_LAST]) { const char *field; int pos, new_pos; @@ -1743,7 +1796,7 @@ decode_header_data (DBusString *data, if (header_len < 16) return FALSE; - + i = 0; while (i < FIELD_LAST) { @@ -1755,9 +1808,14 @@ decode_header_data (DBusString *data, fields[FIELD_BODY_LENGTH].offset = 8; fields[FIELD_CLIENT_SERIAL].offset = 12; - /* Now handle the fields */ + /* Now handle the named fields. A real named field is at least 4 + * bytes for the name, plus a type code (1 byte) plus padding. So + * if we have less than 8 bytes left, it must be alignment padding, + * not a field. While >= 8 bytes can't be entirely alignment + * padding. + */ pos = 16; - while (pos < header_len) + while ((pos + 7) < header_len) { pos = _DBUS_ALIGN_VALUE (pos, 4); @@ -1833,8 +1891,7 @@ decode_header_data (DBusString *data, field[0], field[1], field[2], field[3]); } - /* This function has to validate the fields */ - if (!_dbus_marshal_get_field_end_pos (data, byte_order, pos, &new_pos)) + if (!_dbus_marshal_validate_arg (data, byte_order, pos, &new_pos)) return FALSE; if (new_pos > header_len) @@ -1842,6 +1899,19 @@ decode_header_data (DBusString *data, pos = new_pos; } + + if (pos < header_len) + { + /* Alignment padding, verify that it's nul */ + _dbus_assert ((header_len - pos) < 8); + + if (!_dbus_string_validate_nul (data, + pos, (header_len - pos))) + { + _dbus_verbose ("header alignment padding is not nul\n"); + return FALSE; + } + } return TRUE; } @@ -1893,6 +1963,22 @@ _dbus_message_loader_return_buffer (DBusMessageLoader *loader, header_len = _dbus_unpack_int32 (byte_order, header_data + 4); body_len = _dbus_unpack_int32 (byte_order, header_data + 8); + if (header_len < 16 || body_len < 0) + { + _dbus_verbose ("Message had broken too-small header or body len %d %d\n", + header_len, body_len); + loader->corrupted = TRUE; + return; + } + + if (_DBUS_ALIGN_VALUE (header_len, 8) != (unsigned int) header_len) + { + _dbus_verbose ("header length %d is not aligned to 8 bytes\n", + header_len); + loader->corrupted = TRUE; + return; + } + if (header_len + body_len > loader->max_message_size) { _dbus_verbose ("Message claimed length header = %d body = %d exceeds max message length %d\n", @@ -1905,22 +1991,47 @@ _dbus_message_loader_return_buffer (DBusMessageLoader *loader, { HeaderField fields[FIELD_LAST]; int i; - - /* FIXME right now if this doesn't have enough memory, the - * loader becomes corrupted. Instead we should just not - * parse this message for now. - */ + int next_arg; + if (!decode_header_data (&loader->data, header_len, byte_order, fields)) { loader->corrupted = TRUE; return; } + + next_arg = header_len; + while (next_arg < (header_len + body_len)) + { + int prev = next_arg; + + if (!_dbus_marshal_validate_arg (&loader->data, + byte_order, + next_arg, + &next_arg)) + { + loader->corrupted = TRUE; + return; + } + + _dbus_assert (next_arg > prev); + } + + if (next_arg > (header_len + body_len)) + { + _dbus_verbose ("end of last arg at %d but message has len %d+%d=%d\n", + next_arg, header_len, body_len, + header_len + body_len); + loader->corrupted = TRUE; + return; + } message = dbus_message_new_empty_header (); if (message == NULL) break; /* ugh, postpone this I guess. */ + message->byte_order = byte_order; + /* Copy in the offsets we found */ i = 0; while (i < FIELD_LAST) @@ -2012,6 +2123,12 @@ void _dbus_message_loader_set_max_message_size (DBusMessageLoader *loader, long size) { + if (size > MAX_SANE_MESSAGE_SIZE) + { + _dbus_verbose ("clamping requested max message size %ld to %d\n", + size, MAX_SANE_MESSAGE_SIZE); + size = MAX_SANE_MESSAGE_SIZE; + } loader->max_message_size = size; } diff --git a/dbus/dbus-string.c b/dbus/dbus-string.c index 9acf5cf..f453dcb 100644 --- a/dbus/dbus-string.c +++ b/dbus/dbus-string.c @@ -608,7 +608,7 @@ _dbus_string_align_length (DBusString *str, int delta; DBUS_STRING_PREAMBLE (str); _dbus_assert (alignment >= 1); - _dbus_assert (alignment <= 16); /* arbitrary */ + _dbus_assert (alignment <= 8); /* it has to be a bug if > 8 */ new_len = _DBUS_ALIGN_VALUE (real->len, alignment); @@ -1843,6 +1843,65 @@ _dbus_string_validate_ascii (const DBusString *str, return TRUE; } +/** + * Checks that the given range of the string + * is valid UTF-8. If the given range is not contained + * in the string, returns #FALSE. If the string + * contains any nul bytes in the given range, returns + * #FALSE. + * + * @todo right now just calls _dbus_string_validate_ascii() + * + * @param str the string + * @param start first byte index to check + * @param len number of bytes to check + * @returns #TRUE if the byte range exists and is all valid UTF-8 + */ +dbus_bool_t +_dbus_string_validate_utf8 (const DBusString *str, + int start, + int len) +{ + /* FIXME actually validate UTF-8 */ + return _dbus_string_validate_ascii (str, start, len); +} + +/** + * Checks that the given range of the string + * is all nul bytes. If the given range is + * not contained in the string, returns #FALSE. + * + * @param str the string + * @param start first byte index to check + * @param len number of bytes to check + * @returns #TRUE if the byte range exists and is all nul bytes + */ +dbus_bool_t +_dbus_string_validate_nul (const DBusString *str, + int start, + int len) +{ + const unsigned char *s; + const unsigned char *end; + DBUS_CONST_STRING_PREAMBLE (str); + _dbus_assert (start >= 0); + _dbus_assert (len >= 0); + + if ((start + len) > real->len) + return FALSE; + + s = real->str + start; + end = s + len; + while (s != end) + { + if (*s != '\0') + return FALSE; + ++s; + } + + return TRUE; +} + /** @} */ #ifdef DBUS_BUILD_TESTS diff --git a/dbus/dbus-string.h b/dbus/dbus-string.h index c762a68..e71f7fe 100644 --- a/dbus/dbus-string.h +++ b/dbus/dbus-string.h @@ -181,6 +181,14 @@ dbus_bool_t _dbus_string_validate_ascii (const DBusString *str, int start, int len); +dbus_bool_t _dbus_string_validate_utf8 (const DBusString *str, + int start, + int len); + +dbus_bool_t _dbus_string_validate_nul (const DBusString *str, + int start, + int len); + DBUS_END_DECLS; #endif /* DBUS_STRING_H */ diff --git a/test/data/valid-messages/opposite-endian.message b/test/data/valid-messages/opposite-endian.message index 864795b..fb65d1d 100644 --- a/test/data/valid-messages/opposite-endian.message +++ b/test/data/valid-messages/opposite-endian.message @@ -5,7 +5,7 @@ OPPOSITE_ENDIAN ## VALID_HEADER includes a LENGTH Header and LENGTH Body VALID_HEADER -FIELD_NAME repl +FIELD_NAME rply TYPE INT32 INT32 10000 @@ -17,6 +17,7 @@ FIELD_NAME unkn TYPE INT32 INT32 0xfeeb +ALIGN 8 END_LENGTH Header START_LENGTH Body diff --git a/test/data/valid-messages/simplest-manual.message b/test/data/valid-messages/simplest-manual.message index bf5ddc5..11dce5c 100644 --- a/test/data/valid-messages/simplest-manual.message +++ b/test/data/valid-messages/simplest-manual.message @@ -9,6 +9,7 @@ LENGTH Header LENGTH Body ## client serial INT32 7 +ALIGN 8 END_LENGTH Header START_LENGTH Body END_LENGTH Body diff --git a/test/data/valid-messages/simplest.message b/test/data/valid-messages/simplest.message index 872a58a..a0283aa 100644 --- a/test/data/valid-messages/simplest.message +++ b/test/data/valid-messages/simplest.message @@ -2,6 +2,7 @@ ## VALID_HEADER includes a LENGTH Header and LENGTH Body VALID_HEADER +ALIGN 8 END_LENGTH Header START_LENGTH Body END_LENGTH Body -- 2.7.4