Hi release team! A potential local DoS attack was found in dbus, which can lead to a crash of the bus damon due to infinite recursion in the message validation. The relevant CVE is CVE-2010-4352 , see http://www.remlab.net/op/dbus-variant-recursion.shtml for more details. For experimental the bug was fixed in 1.4.1 and I uploaded an updated package already. The package in unstable/testing (1.2.24-3) is also affected. Upstream has released 1.2.26 including the fix. The complete changelog is: D-Bus 1.2.26 (21 December 2010) == • Fix for CVE-2010-4352: sending messages with excessively-nested variants can crash the bus. The existing restriction to 64-levels of nesting previously only applied to the static type signature; now it also applies to dynamic nesting using variants. Thanks to Rémi Denis-Courmont for discoving this issue. • Corrected thread problem causing some calls to hang for 25s • Enable address reuse on TCP sockets • Fix use of $servicename in init script I've attached the complete debdiff for 1.2.26. I have to add that we already backported the fix for kqueue set_watched_dirs from 1.2.x, so the actual diff is a bit smaller. See also http://cgit.freedesktop.org/dbus/dbus/log/?h=dbus-1.2 Please let me know, if you prefer a 1.2.24-4 upload with a cherry-pick of only the CVE fix or if a 1.2.26-1 upload including the other fixes, which seem reasonable and relatively small, would be ok. Cheers, Michael -- Why is it that all of the instruments seeking intelligent life in the universe are pointed away from Earth?
NEWS | 11 ++++++ bus/config-parser-trivial.c | 19 +++++++++++ bus/config-parser-trivial.h | 3 + bus/dir-watch-kqueue.c | 2 - bus/messagebus.in | 2 - bus/rc.messagebus.in | 2 - configure.in | 6 +-- dbus/dbus-connection-internal.h | 1 dbus/dbus-connection.c | 60 +++++++++++++++++++++++++++++++++++- dbus/dbus-marshal-validate.c | 30 +++++++++++++++--- dbus/dbus-marshal-validate.h | 1 dbus/dbus-message-factory.c | 65 ++++++++++++++++++++++++++++++++++++++++ dbus/dbus-sysdeps-unix.c | 20 +++++++++++- doc/dbus-specification.xml | 14 ++++---- 14 files changed, 216 insertions(+), 20 deletions(-) diff --git a/NEWS b/NEWS index e69de29..3bee8c4 100644 --- a/NEWS +++ b/NEWS @@ -0,0 +1,11 @@ +D-Bus 1.2.26 (21 December 2010) +== + +� Fix for CVE-2010-4352: sending messages with excessively-nested variants can + crash the bus. The existing restriction to 64-levels of nesting previously + only applied to the static type signature; now it also applies to dynamic + nesting using variants. Thanks to Rémi Denis-Courmont for discoving this + issue. +� Corrected thread problem causing some calls to hang for 25s +� Enable address reuse on TCP sockets +� Fix use of $servicename in init script diff --git a/bus/config-parser-trivial.c b/bus/config-parser-trivial.c index fd016a8..8a1f504 100644 --- a/bus/config-parser-trivial.c +++ b/bus/config-parser-trivial.c @@ -131,6 +131,25 @@ bus_config_parser_unref (BusConfigParser *parser) } dbus_bool_t +bus_config_parser_check_doctype (BusConfigParser *parser, + const char *doctype, + DBusError *error) +{ + _DBUS_ASSERT_ERROR_IS_CLEAR (error); + + if (strcmp (doctype, "busconfig") != 0) + { + dbus_set_error (error, + DBUS_ERROR_FAILED, + "Configuration file has the wrong document type %s", + doctype); + return FALSE; + } + else + return TRUE; +} + +dbus_bool_t bus_config_parser_start_element (BusConfigParser *parser, const char *element_name, const char **attribute_names, diff --git a/bus/config-parser-trivial.h b/bus/config-parser-trivial.h index ce542bf..6733b1f 100644 --- a/bus/config-parser-trivial.h +++ b/bus/config-parser-trivial.h @@ -41,6 +41,9 @@ BusConfigParser* bus_config_parser_new (const DBusString *basedir, BusConfigParser* bus_config_parser_ref (BusConfigParser *parser); void bus_config_parser_unref (BusConfigParser *parser); +dbus_bool_t bus_config_parser_check_doctype (BusConfigParser *parser, + const char *doctype, + DBusError *error); dbus_bool_t bus_config_parser_start_element (BusConfigParser *parser, const char *element_name, const char **attribute_names, diff --git a/bus/dir-watch-kqueue.c b/bus/dir-watch-kqueue.c index 4a01b74..4e436eb 100644 --- a/bus/dir-watch-kqueue.c +++ b/bus/dir-watch-kqueue.c @@ -169,7 +169,7 @@ bus_set_watched_dirs (BusContext *context, DBusList **directories) */ for (i = 0; new_dirs[i]; i++) { - for (j = 0; i < num_fds; j++) + for (j = 0; j < num_fds; j++) { if (dirs[j] && strcmp (new_dirs[i], dirs[j]) == 0) { diff --git a/bus/messagebus.in b/bus/messagebus.in index 1f1004b..3e2ee07 100755 --- a/bus/messagebus.in +++ b/bus/messagebus.in @@ -68,7 +68,7 @@ case "$1" in stop ;; status) - status $processname + status $servicename RETVAL=$? ;; restart) diff --git a/bus/rc.messagebus.in b/bus/rc.messagebus.in index b147503..c52ca77 100644 --- a/bus/rc.messagebus.in +++ b/bus/rc.messagebus.in @@ -61,7 +61,7 @@ case "$1" in stop ;; status) - status $processname + status $servicename RETVAL=$? ;; restart) diff --git a/configure.in b/configure.in index 486c612..a57ba9a 100644 --- a/configure.in +++ b/configure.in @@ -3,7 +3,7 @@ AC_PREREQ(2.52) m4_define([dbus_major_version], [1]) m4_define([dbus_minor_version], [2]) -m4_define([dbus_micro_version], [24]) +m4_define([dbus_micro_version], [26]) m4_define([dbus_version], [dbus_major_version.dbus_minor_version.dbus_micro_version]) AC_INIT(dbus, [dbus_version]) @@ -35,7 +35,7 @@ LT_CURRENT=7 ## increment any time the source changes; set to ## 0 if you increment CURRENT -LT_REVISION=0 +LT_REVISION=1 ## increment if any interfaces have been added; set to 0 ## if any interfaces have been changed or removed. removal has @@ -81,7 +81,7 @@ AC_ARG_ENABLE(kqueue, AS_HELP_STRING([--enable-kqueue],[build with kqueue suppor AC_ARG_ENABLE(console-owner-file, AS_HELP_STRING([--enable-console-owner-file],[enable console owner file]),enable_console_owner_file=$enableval,enable_console_owner_file=auto) AC_ARG_ENABLE(userdb-cache, AS_HELP_STRING([--enable-userdb-cache],[build with userdb-cache support]),enable_userdb_cache=$enableval,enable_userdb_cache=yes) -AC_ARG_WITH(xml, AS_HELP_STRING([--with-xml=[libxml/expat]],[XML library to use])) +AC_ARG_WITH(xml, AS_HELP_STRING([--with-xml=[libxml/expat]],[XML library to use (libxml may be named libxml2 on some systems)])) AC_ARG_WITH(init-scripts, AS_HELP_STRING([--with-init-scripts=[redhat]],[Style of init scripts to install])) AC_ARG_WITH(session-socket-dir, AS_HELP_STRING([--with-session-socket-dir=[dirname]],[Where to put sockets for the per-login-session message bus])) AC_ARG_WITH(test-socket-dir, AS_HELP_STRING([--with-test-socket-dir=[dirname]],[Where to put sockets for make check])) diff --git a/dbus/dbus-connection-internal.h b/dbus/dbus-connection-internal.h index 721b5d7..cdf3f59 100644 --- a/dbus/dbus-connection-internal.h +++ b/dbus/dbus-connection-internal.h @@ -75,6 +75,7 @@ void _dbus_connection_toggle_timeout_unlocked (DBusConnection dbus_bool_t enabled); DBusConnection* _dbus_connection_new_for_transport (DBusTransport *transport); void _dbus_connection_do_iteration_unlocked (DBusConnection *connection, + DBusPendingCall *pending, unsigned int flags, int timeout_milliseconds); void _dbus_connection_close_possibly_shared (DBusConnection *connection); diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c index 9526d3c..b3cfa3d 100644 --- a/dbus/dbus-connection.c +++ b/dbus/dbus-connection.c @@ -320,6 +320,8 @@ static void _dbus_connection_release_dispatch (DB static DBusDispatchStatus _dbus_connection_flush_unlocked (DBusConnection *connection); static void _dbus_connection_close_possibly_shared_and_unlock (DBusConnection *connection); static dbus_bool_t _dbus_connection_get_is_connected_unlocked (DBusConnection *connection); +static dbus_bool_t _dbus_connection_peek_for_reply_unlocked (DBusConnection *connection, + dbus_uint32_t client_serial); static DBusMessageFilter * _dbus_message_filter_ref (DBusMessageFilter *filter) @@ -1137,14 +1139,22 @@ _dbus_connection_release_io_path (DBusConnection *connection) * you specify DBUS_ITERATION_BLOCK; in that case the function * returns immediately. * + * If pending is not NULL then a check is made if the pending call + * is completed after the io path has been required. If the call + * has been completed nothing is done. This must be done since + * the _dbus_connection_acquire_io_path releases the connection + * lock for a while. + * * Called with connection lock held. * * @param connection the connection. + * @param pending the pending call that should be checked or NULL * @param flags iteration flags. * @param timeout_milliseconds maximum blocking time, or -1 for no limit. */ void _dbus_connection_do_iteration_unlocked (DBusConnection *connection, + DBusPendingCall *pending, unsigned int flags, int timeout_milliseconds) { @@ -1160,8 +1170,22 @@ _dbus_connection_do_iteration_unlocked (DBusConnection *connection, { HAVE_LOCK_CHECK (connection); - _dbus_transport_do_iteration (connection->transport, - flags, timeout_milliseconds); + if ( (pending != NULL) && _dbus_pending_call_get_completed_unlocked(pending)) + { + _dbus_verbose ("pending call completed while acquiring I/O path"); + } + else if ( (pending != NULL) && + _dbus_connection_peek_for_reply_unlocked (connection, + _dbus_pending_call_get_reply_serial_unlocked (pending))) + { + _dbus_verbose ("pending call completed while acquiring I/O path (reply found in queue)"); + } + else + { + _dbus_transport_do_iteration (connection->transport, + flags, timeout_milliseconds); + } + _dbus_connection_release_io_path (connection); } @@ -1989,6 +2013,7 @@ _dbus_connection_send_preallocated_unlocked_no_update (DBusConnection *con * out immediately, and otherwise get them queued up */ _dbus_connection_do_iteration_unlocked (connection, + NULL, DBUS_ITERATION_DO_WRITING, -1); @@ -2157,6 +2182,32 @@ generate_local_error_message (dbus_uint32_t serial, return message; } +/* + * Peek the incoming queue to see if we got reply for a specific serial + */ +static dbus_bool_t +_dbus_connection_peek_for_reply_unlocked (DBusConnection *connection, + dbus_uint32_t client_serial) +{ + DBusList *link; + HAVE_LOCK_CHECK (connection); + + link = _dbus_list_get_first_link (&connection->incoming_messages); + + while (link != NULL) + { + DBusMessage *reply = link->data; + + if (dbus_message_get_reply_serial (reply) == client_serial) + { + _dbus_verbose ("%s reply to %d found in queue\n", _DBUS_FUNCTION_NAME, client_serial); + return TRUE; + } + link = _dbus_list_get_next_link (&connection->incoming_messages, link); + } + + return FALSE; +} /* This is slightly strange since we can pop a message here without * the dispatch lock. @@ -2333,6 +2384,7 @@ _dbus_connection_block_pending_call (DBusPendingCall *pending) /* Now we wait... */ /* always block at least once as we know we don't have the reply yet */ _dbus_connection_do_iteration_unlocked (connection, + pending, DBUS_ITERATION_DO_READING | DBUS_ITERATION_BLOCK, timeout_milliseconds); @@ -2399,6 +2451,7 @@ _dbus_connection_block_pending_call (DBusPendingCall *pending) { /* block again, we don't have the reply buffered yet. */ _dbus_connection_do_iteration_unlocked (connection, + pending, DBUS_ITERATION_DO_READING | DBUS_ITERATION_BLOCK, timeout_milliseconds - elapsed_milliseconds); @@ -2426,6 +2479,7 @@ _dbus_connection_block_pending_call (DBusPendingCall *pending) { /* block again, we don't have the reply buffered yet. */ _dbus_connection_do_iteration_unlocked (connection, + NULL, DBUS_ITERATION_DO_READING | DBUS_ITERATION_BLOCK, timeout_milliseconds - elapsed_milliseconds); @@ -3403,6 +3457,7 @@ _dbus_connection_flush_unlocked (DBusConnection *connection) _dbus_verbose ("doing iteration in %s\n", _DBUS_FUNCTION_NAME); HAVE_LOCK_CHECK (connection); _dbus_connection_do_iteration_unlocked (connection, + NULL, DBUS_ITERATION_DO_READING | DBUS_ITERATION_DO_WRITING | DBUS_ITERATION_BLOCK, @@ -3489,6 +3544,7 @@ _dbus_connection_read_write_dispatch (DBusConnection *connection, { _dbus_verbose ("doing iteration in %s\n", _DBUS_FUNCTION_NAME); _dbus_connection_do_iteration_unlocked (connection, + NULL, DBUS_ITERATION_DO_READING | DBUS_ITERATION_DO_WRITING | DBUS_ITERATION_BLOCK, diff --git a/dbus/dbus-marshal-validate.c b/dbus/dbus-marshal-validate.c index 61fad4d..c681595 100644 --- a/dbus/dbus-marshal-validate.c +++ b/dbus/dbus-marshal-validate.c @@ -289,16 +289,30 @@ out: return result; } +/* note: this function is also used to validate the header's values, + * since the header is a valid body with a particular signature. + */ static DBusValidity validate_body_helper (DBusTypeReader *reader, int byte_order, dbus_bool_t walk_reader_to_end, + int total_depth, const unsigned char *p, const unsigned char *end, const unsigned char **new_p) { int current_type; + /* The spec allows arrays and structs to each nest 32, for total + * nesting of 2*32. We want to impose the same limit on "dynamic" + * value nesting (not visible in the signature) which is introduced + * by DBUS_TYPE_VARIANT. + */ + if (total_depth > (DBUS_MAXIMUM_TYPE_RECURSION_DEPTH * 2)) + { + return DBUS_INVALID_NESTED_TOO_DEEPLY; + } + while ((current_type = _dbus_type_reader_get_current_type (reader)) != DBUS_TYPE_INVALID) { const unsigned char *a; @@ -474,7 +488,9 @@ validate_body_helper (DBusTypeReader *reader, { while (p < array_end) { - validity = validate_body_helper (&sub, byte_order, FALSE, p, end, &p); + validity = validate_body_helper (&sub, byte_order, FALSE, + total_depth + 1, + p, end, &p); if (validity != DBUS_VALID) return validity; } @@ -591,7 +607,9 @@ validate_body_helper (DBusTypeReader *reader, _dbus_assert (_dbus_type_reader_get_current_type (&sub) != DBUS_TYPE_INVALID); - validity = validate_body_helper (&sub, byte_order, FALSE, p, end, &p); + validity = validate_body_helper (&sub, byte_order, FALSE, + total_depth + 1, + p, end, &p); if (validity != DBUS_VALID) return validity; @@ -620,7 +638,9 @@ validate_body_helper (DBusTypeReader *reader, _dbus_type_reader_recurse (reader, &sub); - validity = validate_body_helper (&sub, byte_order, TRUE, p, end, &p); + validity = validate_body_helper (&sub, byte_order, TRUE, + total_depth + 1, + p, end, &p); if (validity != DBUS_VALID) return validity; } @@ -705,7 +725,7 @@ _dbus_validate_body_with_reason (const DBusString *expected_signature, p = _dbus_string_get_const_data_len (value_str, value_pos, len); end = p + len; - validity = validate_body_helper (&reader, byte_order, TRUE, p, end, &p); + validity = validate_body_helper (&reader, byte_order, TRUE, 0, p, end, &p); if (validity != DBUS_VALID) return validity; @@ -875,7 +895,7 @@ _dbus_validity_to_error_message (DBusValidity validity) case DBUS_INVALID_DICT_ENTRY_HAS_TOO_MANY_FIELDS: return "Dict entry has too many fields"; case DBUS_INVALID_DICT_ENTRY_NOT_INSIDE_ARRAY: return "Dict entry not inside array"; case DBUS_INVALID_DICT_KEY_MUST_BE_BASIC_TYPE: return "Dict key must be basic type"; - + case DBUS_INVALID_NESTED_TOO_DEEPLY: return "Variants cannot be used to create a hugely recursive tree of values"; default: return "Invalid"; } diff --git a/dbus/dbus-marshal-validate.h b/dbus/dbus-marshal-validate.h index a7d904b..da599c6 100644 --- a/dbus/dbus-marshal-validate.h +++ b/dbus/dbus-marshal-validate.h @@ -117,6 +117,7 @@ typedef enum DBUS_INVALID_DICT_ENTRY_HAS_TOO_MANY_FIELDS = 53, DBUS_INVALID_DICT_ENTRY_NOT_INSIDE_ARRAY = 54, DBUS_INVALID_DICT_KEY_MUST_BE_BASIC_TYPE = 55, + DBUS_INVALID_NESTED_TOO_DEEPLY = 56, DBUS_VALIDITY_LAST } DBusValidity; diff --git a/dbus/dbus-message-factory.c b/dbus/dbus-message-factory.c index 7bc539b..49151eb 100644 --- a/dbus/dbus-message-factory.c +++ b/dbus/dbus-message-factory.c @@ -333,6 +333,53 @@ simple_error (void) return message; } +static DBusMessage* +message_with_nesting_levels (int levels) +{ + DBusMessage *message; + dbus_int32_t v_INT32; + DBusMessageIter *parents; + DBusMessageIter *children; + int i; + + /* If levels is higher it breaks sig_refcount in DBusMessageRealIter + * in dbus-message.c, this assert is just to help you know you need + * to fix that if you hit it + */ + _dbus_assert (levels < 256); + + parents = dbus_new(DBusMessageIter, levels + 1); + children = dbus_new(DBusMessageIter, levels + 1); + + v_INT32 = 42; + message = simple_method_call (); + + i = 0; + dbus_message_iter_init_append (message, &parents[i]); + while (i < levels) + { + dbus_message_iter_open_container (&parents[i], DBUS_TYPE_VARIANT, + i == (levels - 1) ? + DBUS_TYPE_INT32_AS_STRING : + DBUS_TYPE_VARIANT_AS_STRING, + &children[i]); + ++i; + parents[i] = children[i-1]; + } + --i; + dbus_message_iter_append_basic (&children[i], DBUS_TYPE_INT32, &v_INT32); + while (i >= 0) + { + dbus_message_iter_close_container (&parents[i], &children[i]); + --i; + } + + dbus_free(parents); + dbus_free(children); + + return message; +} + static dbus_bool_t generate_special (DBusMessageDataIter *iter, DBusString *data, @@ -735,6 +782,24 @@ generate_special (DBusMessageDataIter *iter, *expected_validity = DBUS_INVALID_DICT_ENTRY_HAS_NO_FIELDS; } + else if (item_seq == 20) + { + /* 64 levels of nesting is OK */ + message = message_with_nesting_levels(64); + + generate_from_message (data, expected_validity, message); + + *expected_validity = DBUS_VALID; + } + else if (item_seq == 21) + { + /* 65 levels of nesting is not OK */ + message = message_with_nesting_levels(65); + + generate_from_message (data, expected_validity, message); + + *expected_validity = DBUS_INVALID_NESTED_TOO_DEEPLY; + } else { return FALSE; diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c index ce3475a..b58d09a 100644 --- a/dbus/dbus-sysdeps-unix.c +++ b/dbus/dbus-sysdeps-unix.c @@ -623,6 +623,7 @@ _dbus_listen_unix_socket (const char *path, int listen_fd; struct sockaddr_un addr; size_t path_len; + unsigned int reuseaddr; _DBUS_ASSERT_ERROR_IS_CLEAR (error); @@ -696,7 +697,15 @@ _dbus_listen_unix_socket (const char *path, strncpy (addr.sun_path, path, path_len); } - + + reuseaddr = 1; + if (setsockopt (listen_fd, SOL_SOCKET, SO_REUSEADDR, &reuseaddr, sizeof(reuseaddr))==-1) + { + dbus_set_error (error, _dbus_error_from_errno (errno), + "Failed to set socket option\"%s\": %s", + path, _dbus_strerror (errno)); + } + if (bind (listen_fd, (struct sockaddr*) &addr, _DBUS_STRUCT_OFFSET (struct sockaddr_un, sun_path) + path_len) < 0) { dbus_set_error (error, _dbus_error_from_errno (errno), @@ -870,6 +879,7 @@ _dbus_listen_tcp_socket (const char *host, int nlisten_fd = 0, *listen_fd = NULL, res, i; struct addrinfo hints; struct addrinfo *ai, *tmp; + unsigned int reuseaddr; *fds_p = NULL; _DBUS_ASSERT_ERROR_IS_CLEAR (error); @@ -915,6 +925,14 @@ _dbus_listen_tcp_socket (const char *host, } _DBUS_ASSERT_ERROR_IS_CLEAR(error); + reuseaddr = 1; + if (setsockopt (fd, SOL_SOCKET, SO_REUSEADDR, &reuseaddr, sizeof(reuseaddr))==-1) + { + dbus_set_error (error, _dbus_error_from_errno (errno), + "Failed to set socket option \"%s:%s\": %s", + host ? host : "*", port, _dbus_strerror (errno)); + } + if (bind (fd, (struct sockaddr*) tmp->ai_addr, tmp->ai_addrlen) < 0) { _dbus_close(fd, NULL); diff --git a/doc/dbus-specification.xml b/doc/dbus-specification.xml index b5866e5..7793016 100644 --- a/doc/dbus-specification.xml +++ b/doc/dbus-specification.xml @@ -561,12 +561,14 @@ </row><row> <entry><literal>VARIANT</literal></entry> <entry> - A variant type has a marshaled <literal>SIGNATURE</literal> - followed by a marshaled value with the type - given in the signature. - Unlike a message signature, the variant signature - can contain only a single complete type. - So "i", "ai" or "(ii)" is OK, but "ii" is not. + A variant type has a marshaled + <literal>SIGNATURE</literal> followed by a marshaled + value with the type given in the signature. Unlike + a message signature, the variant signature can + contain only a single complete type. So "i", "ai" + or "(ii)" is OK, but "ii" is not. Use of variants may not + cause a total message depth to be larger than 64, including + other container types such as structures. </entry> <entry> 1 (alignment of the signature)
Attachment:
signature.asc
Description: OpenPGP digital signature