From 1d14348e705474cd0632cb52810a906a8fbdbad9 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 12 May 2015 11:35:04 +0100 Subject: [PATCH 4/5] Make UUID generation failable Previously, this would always succeed, but might use weak random numbers in rare failure cases. I don't think these UUIDs are security-sensitive, but if they're generated by a PRNG as weak as rand() (<= 32 bits of entropy), we certainly can't claim that they're universally unique. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=90414 --- bus/bus.c | 3 ++- dbus/dbus-connection.c | 38 ++++++++++++++++++++++++++++++++------ dbus/dbus-internals.c | 40 ++++++++++++++++++++++++++++------------ dbus/dbus-internals.h | 8 +++++--- dbus/dbus-misc.c | 6 +++++- dbus/dbus-server.c | 4 +++- dbus/dbus-sysdeps-unix.c | 8 +++++--- dbus/dbus-uuidgen.c | 15 +++++++-------- dbus/dbus-uuidgen.h | 4 ++-- tools/dbus-uuidgen.c | 7 ++----- 10 files changed, 91 insertions(+), 42 deletions(-) diff --git a/bus/bus.c b/bus/bus.c index 68de1b2..1aa893b 100644 --- a/bus/bus.c +++ b/bus/bus.c @@ -766,7 +766,8 @@ bus_context_new (const DBusString *config_file, } context->refcount = 1; - _dbus_generate_uuid (&context->uuid); + if (!_dbus_generate_uuid (&context->uuid, error)) + goto failed; if (!_dbus_string_copy_data (config_file, &context->config_file)) { diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c index 8952b75..97512fb 100644 --- a/dbus/dbus-connection.c +++ b/dbus/dbus-connection.c @@ -4436,15 +4436,24 @@ _dbus_connection_peer_filter_unlocked_no_update (DBusConnection *connection, "GetMachineId")) { DBusString uuid; - - ret = dbus_message_new_method_return (message); - if (ret == NULL) + DBusError error = DBUS_ERROR_INIT; + + if (!_dbus_string_init (&uuid)) goto out; - _dbus_string_init (&uuid); - if (_dbus_get_local_machine_uuid_encoded (&uuid)) + if (_dbus_get_local_machine_uuid_encoded (&uuid, &error)) { - const char *v_STRING = _dbus_string_get_const_data (&uuid); + const char *v_STRING; + + ret = dbus_message_new_method_return (message); + + if (ret == NULL) + { + _dbus_string_free (&uuid); + goto out; + } + + v_STRING = _dbus_string_get_const_data (&uuid); if (dbus_message_append_args (ret, DBUS_TYPE_STRING, &v_STRING, DBUS_TYPE_INVALID)) @@ -4452,6 +4461,23 @@ _dbus_connection_peer_filter_unlocked_no_update (DBusConnection *connection, sent = _dbus_connection_send_unlocked_no_update (connection, ret, NULL); } } + else if (dbus_error_has_name (&error, DBUS_ERROR_NO_MEMORY)) + { + dbus_error_free (&error); + goto out; + } + else + { + ret = dbus_message_new_error (message, error.name, error.message); + dbus_error_free (&error); + + if (ret == NULL) + goto out; + + sent = _dbus_connection_send_unlocked_no_update (connection, ret, + NULL); + } + _dbus_string_free (&uuid); } else diff --git a/dbus/dbus-internals.c b/dbus/dbus-internals.c index c7f81e5..d988d59 100644 --- a/dbus/dbus-internals.c +++ b/dbus/dbus-internals.c @@ -647,8 +647,9 @@ _dbus_string_array_contains (const char **array, * * @param uuid the uuid to initialize */ -void -_dbus_generate_uuid (DBusGUID *uuid) +dbus_bool_t +_dbus_generate_uuid (DBusGUID *uuid, + DBusError *error) { long now; @@ -660,6 +661,7 @@ _dbus_generate_uuid (DBusGUID *uuid) uuid->as_uint32s[DBUS_UUID_LENGTH_WORDS - 1] = DBUS_UINT32_TO_BE (now); _dbus_generate_random_bytes_buffer (uuid->as_bytes, DBUS_UUID_LENGTH_BYTES - 4); + return TRUE; } /** @@ -841,7 +843,10 @@ _dbus_read_uuid_file (const DBusString *filename, else { dbus_error_free (&read_error); - _dbus_generate_uuid (uuid); + + if (!_dbus_generate_uuid (uuid, error)) + return FALSE; + return _dbus_write_uuid_file (filename, uuid, error); } } @@ -861,19 +866,23 @@ static DBusGUID machine_uuid; * @returns #FALSE if no memory */ dbus_bool_t -_dbus_get_local_machine_uuid_encoded (DBusString *uuid_str) +_dbus_get_local_machine_uuid_encoded (DBusString *uuid_str, + DBusError *error) { - dbus_bool_t ok; + dbus_bool_t ok = TRUE; if (!_DBUS_LOCK (machine_uuid)) - return FALSE; + { + _DBUS_SET_OOM (error); + return FALSE; + } if (machine_uuid_initialized_generation != _dbus_current_generation) { - DBusError error = DBUS_ERROR_INIT; + DBusError local_error = DBUS_ERROR_INIT; if (!_dbus_read_local_machine_uuid (&machine_uuid, FALSE, - &error)) + &local_error)) { #ifndef DBUS_ENABLE_EMBEDDED_TESTS /* For the test suite, we may not be installed so just continue silently @@ -882,16 +891,23 @@ _dbus_get_local_machine_uuid_encoded (DBusString *uuid_str) */ _dbus_warn_check_failed ("D-Bus library appears to be incorrectly set up; failed to read machine uuid: %s\n" "See the manual page for dbus-uuidgen to correct this issue.\n", - error.message); + local_error.message); #endif - dbus_error_free (&error); + dbus_error_free (&local_error); - _dbus_generate_uuid (&machine_uuid); + ok = _dbus_generate_uuid (&machine_uuid, error); } } - ok = _dbus_uuid_encode (&machine_uuid, uuid_str); + if (ok) + { + if (!_dbus_uuid_encode (&machine_uuid, uuid_str)) + { + ok = FALSE; + _DBUS_SET_OOM (error); + } + } _DBUS_UNLOCK (machine_uuid); diff --git a/dbus/dbus-internals.h b/dbus/dbus-internals.h index 6368538..a0d69c2 100644 --- a/dbus/dbus-internals.h +++ b/dbus/dbus-internals.h @@ -392,8 +392,9 @@ union DBusGUID char as_bytes[DBUS_UUID_LENGTH_BYTES]; /**< guid as 16 single-byte values */ }; -DBUS_PRIVATE_EXPORT -void _dbus_generate_uuid (DBusGUID *uuid); +DBUS_PRIVATE_EXPORT _DBUS_GNUC_WARN_UNUSED_RESULT +dbus_bool_t _dbus_generate_uuid (DBusGUID *uuid, + DBusError *error); DBUS_PRIVATE_EXPORT dbus_bool_t _dbus_uuid_encode (const DBusGUID *uuid, DBusString *encoded); @@ -406,7 +407,8 @@ dbus_bool_t _dbus_write_uuid_file (const DBusString *filename, const DBusGUID *uuid, DBusError *error); -dbus_bool_t _dbus_get_local_machine_uuid_encoded (DBusString *uuid_str); +dbus_bool_t _dbus_get_local_machine_uuid_encoded (DBusString *uuid_str, + DBusError *error); #define _DBUS_PASTE2(a, b) a ## b #define _DBUS_PASTE(a, b) _DBUS_PASTE2 (a, b) diff --git a/dbus/dbus-misc.c b/dbus/dbus-misc.c index 6ca30f2..a2da562 100644 --- a/dbus/dbus-misc.c +++ b/dbus/dbus-misc.c @@ -80,7 +80,11 @@ dbus_get_local_machine_id (void) if (!_dbus_string_init (&uuid)) return NULL; - if (!_dbus_get_local_machine_uuid_encoded (&uuid) || + /* The documentation says dbus_get_local_machine_id() only fails on OOM; + * this can actually also fail if the D-Bus installation is faulty + * (no UUID) *and* reading a new random UUID fails, but we have no way + * to report that */ + if (!_dbus_get_local_machine_uuid_encoded (&uuid, NULL) || !_dbus_string_steal_data (&uuid, &s)) { _dbus_string_free (&uuid); diff --git a/dbus/dbus-server.c b/dbus/dbus-server.c index d062eb7..12c793a 100644 --- a/dbus/dbus-server.c +++ b/dbus/dbus-server.c @@ -136,7 +136,8 @@ _dbus_server_init_base (DBusServer *server, return FALSE; } - _dbus_generate_uuid (&server->guid); + if (!_dbus_generate_uuid (&server->guid, error)) + goto failed; if (!_dbus_uuid_encode (&server->guid, &server->guid_hex)) goto oom; @@ -166,6 +167,7 @@ _dbus_server_init_base (DBusServer *server, oom: _DBUS_SET_OOM (error); + failed: _dbus_rmutex_free_at_location (&server->mutex); server->mutex = NULL; if (server->watches) diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c index ab7efc2..e8178c8 100644 --- a/dbus/dbus-sysdeps-unix.c +++ b/dbus/dbus-sysdeps-unix.c @@ -3734,9 +3734,8 @@ _dbus_get_autolaunch_address (const char *scope, return FALSE; } - if (!_dbus_get_local_machine_uuid_encoded (&uuid)) + if (!_dbus_get_local_machine_uuid_encoded (&uuid, error)) { - _DBUS_SET_OOM (error); goto out; } @@ -3836,7 +3835,10 @@ _dbus_read_local_machine_uuid (DBusGUID *machine_id, /* if none found, try to make a new one */ dbus_error_free (error); _dbus_string_init_const (&filename, DBUS_MACHINE_UUID_FILE); - _dbus_generate_uuid (machine_id); + + if (!_dbus_generate_uuid (machine_id, error)) + return FALSE; + return _dbus_write_uuid_file (&filename, machine_id, error); } diff --git a/dbus/dbus-uuidgen.c b/dbus/dbus-uuidgen.c index 6d7c0ae..2b0f85b 100644 --- a/dbus/dbus-uuidgen.c +++ b/dbus/dbus-uuidgen.c @@ -111,20 +111,19 @@ dbus_internal_do_not_use_get_uuid (const char *filename, } /** - * For use by the dbus-uuidgen binary ONLY, do not call this. - * We can and will change this function without modifying - * the libdbus soname. - * * @param uuid_p out param to return the uuid - * @returns #FALSE if no memory + * @returns #FALSE on error */ dbus_bool_t -dbus_internal_do_not_use_create_uuid (char **uuid_p) +_dbus_create_uuid (char **uuid_p, + DBusError *error) { DBusGUID uuid; - _dbus_generate_uuid (&uuid); - return return_uuid (&uuid, uuid_p, NULL); + if (!_dbus_generate_uuid (&uuid, error)) + return FALSE; + + return return_uuid (&uuid, uuid_p, error); } /** @} */ diff --git a/dbus/dbus-uuidgen.h b/dbus/dbus-uuidgen.h index 9d12f2b..5e4a948 100644 --- a/dbus/dbus-uuidgen.h +++ b/dbus/dbus-uuidgen.h @@ -41,8 +41,8 @@ dbus_bool_t dbus_internal_do_not_use_ensure_uuid (const char *filename, char **uuid_p, DBusError *error); DBUS_PRIVATE_EXPORT -dbus_bool_t dbus_internal_do_not_use_create_uuid (char **uuid_p); - +dbus_bool_t _dbus_create_uuid (char **uuid_p, + DBusError *error); DBUS_END_DECLS diff --git a/tools/dbus-uuidgen.c b/tools/dbus-uuidgen.c index c8ba1cf..03ce553 100644 --- a/tools/dbus-uuidgen.c +++ b/tools/dbus-uuidgen.c @@ -137,15 +137,12 @@ main (int argc, char *argv[]) else { char *uuid; - if (dbus_internal_do_not_use_create_uuid (&uuid)) + + if (_dbus_create_uuid (&uuid, &error)) { printf ("%s\n", uuid); dbus_free (uuid); } - else - { - dbus_set_error (&error, DBUS_ERROR_NO_MEMORY, "No memory"); - } } if (dbus_error_is_set (&error)) -- 2.1.4