From 545570a91ecf6bb38c1d1ab7c80165efa60523e1 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 12 May 2015 11:47:29 +0100 Subject: [PATCH 5/5] Fail to generate random bytes instead of falling back to rand() This is more robust against broken setups where we run out of memory or cannot read /dev/urandom. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=90414 --- bus/activation.c | 2 +- dbus/dbus-auth.c | 64 ++++++++++++++++++++++++++++++++++-------------- dbus/dbus-file-unix.c | 4 +-- dbus/dbus-file-win.c | 4 +-- dbus/dbus-internals.c | 14 ++++++++++- dbus/dbus-keyring.c | 10 +++----- dbus/dbus-nonce.c | 6 ++--- dbus/dbus-server-unix.c | 20 ++++++++++++--- dbus/dbus-sysdeps-unix.c | 55 ++++++++++++++++++----------------------- dbus/dbus-sysdeps-win.c | 14 ++++++++--- dbus/dbus-sysdeps.c | 51 +++++++++----------------------------- dbus/dbus-sysdeps.h | 14 ++++++----- 12 files changed, 140 insertions(+), 118 deletions(-) diff --git a/bus/activation.c b/bus/activation.c index 390b836..679a40e 100644 --- a/bus/activation.c +++ b/bus/activation.c @@ -2608,7 +2608,7 @@ bus_activation_service_reload_test (const DBusString *test_data_dir) return FALSE; if (!_dbus_string_append (&directory, "/dbus-reload-test-") || - !_dbus_generate_random_ascii (&directory, 6)) + !_dbus_generate_random_ascii (&directory, 6, NULL)) { return FALSE; } diff --git a/dbus/dbus-auth.c b/dbus/dbus-auth.c index 1503d5f..f222787 100644 --- a/dbus/dbus-auth.c +++ b/dbus/dbus-auth.c @@ -524,10 +524,8 @@ sha1_handle_first_client_response (DBusAuth *auth, */ DBusString tmp; DBusString tmp2; - dbus_bool_t retval; - DBusError error; - - retval = FALSE; + dbus_bool_t retval = FALSE; + DBusError error = DBUS_ERROR_INIT; _dbus_string_set_length (&auth->challenge, 0); @@ -578,7 +576,6 @@ sha1_handle_first_client_response (DBusAuth *auth, if (auth->keyring == NULL) { - dbus_error_init (&error); auth->keyring = _dbus_keyring_new_for_credentials (auth->desired_identity, &auth->context, &error); @@ -610,7 +607,6 @@ sha1_handle_first_client_response (DBusAuth *auth, _dbus_assert (auth->keyring != NULL); - dbus_error_init (&error); auth->cookie_id = _dbus_keyring_get_best_key (auth->keyring, &error); if (auth->cookie_id < 0) { @@ -640,8 +636,25 @@ sha1_handle_first_client_response (DBusAuth *auth, if (!_dbus_string_append (&tmp2, " ")) goto out; - if (!_dbus_generate_random_bytes (&tmp, N_CHALLENGE_BYTES)) - goto out; + if (!_dbus_generate_random_bytes (&tmp, N_CHALLENGE_BYTES, &error)) + { + if (dbus_error_has_name (&error, DBUS_ERROR_NO_MEMORY)) + { + dbus_error_free (&error); + goto out; + } + else + { + _DBUS_ASSERT_ERROR_IS_SET (&error); + _dbus_verbose ("%s: Error generating challenge: %s\n", + DBUS_AUTH_NAME (auth), error.message); + if (send_rejected (auth)) + retval = TRUE; /* retval is only about mem */ + + dbus_error_free (&error); + goto out; + } + } _dbus_string_set_length (&auth->challenge, 0); if (!_dbus_string_hex_encode (&tmp, 0, &auth->challenge, 0)) @@ -826,7 +839,7 @@ handle_client_data_cookie_sha1_mech (DBusAuth *auth, * name, the cookie ID, and the server challenge, separated by * spaces. We send back our challenge string and the correct hash. */ - dbus_bool_t retval; + dbus_bool_t retval = FALSE; DBusString context; DBusString cookie_id_str; DBusString server_challenge; @@ -835,9 +848,8 @@ handle_client_data_cookie_sha1_mech (DBusAuth *auth, DBusString tmp; int i, j; long val; - - retval = FALSE; - + DBusError error = DBUS_ERROR_INIT; + if (!_dbus_string_find_blank (data, 0, &i)) { if (send_error (auth, @@ -903,9 +915,6 @@ handle_client_data_cookie_sha1_mech (DBusAuth *auth, if (auth->keyring == NULL) { - DBusError error; - - dbus_error_init (&error); auth->keyring = _dbus_keyring_new_for_credentials (NULL, &context, &error); @@ -942,9 +951,28 @@ handle_client_data_cookie_sha1_mech (DBusAuth *auth, if (!_dbus_string_init (&tmp)) goto out_3; - - if (!_dbus_generate_random_bytes (&tmp, N_CHALLENGE_BYTES)) - goto out_4; + + if (!_dbus_generate_random_bytes (&tmp, N_CHALLENGE_BYTES, &error)) + { + if (dbus_error_has_name (&error, DBUS_ERROR_NO_MEMORY)) + { + dbus_error_free (&error); + goto out_4; + } + else + { + _DBUS_ASSERT_ERROR_IS_SET (&error); + + _dbus_verbose ("%s: Failed to generate challenge: %s\n", + DBUS_AUTH_NAME (auth), error.message); + + if (send_error (auth, "Failed to generate challenge")) + retval = TRUE; /* retval is only about mem */ + + dbus_error_free (&error); + goto out_4; + } + } if (!_dbus_string_init (&client_challenge)) goto out_4; diff --git a/dbus/dbus-file-unix.c b/dbus/dbus-file-unix.c index 1975933..830d3fe 100644 --- a/dbus/dbus-file-unix.c +++ b/dbus/dbus-file-unix.c @@ -202,9 +202,9 @@ _dbus_string_save_to_file (const DBusString *str, } #define N_TMP_FILENAME_RANDOM_BYTES 8 - if (!_dbus_generate_random_ascii (&tmp_filename, N_TMP_FILENAME_RANDOM_BYTES)) + if (!_dbus_generate_random_ascii (&tmp_filename, N_TMP_FILENAME_RANDOM_BYTES, + error)) { - dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL); _dbus_string_free (&tmp_filename); return FALSE; } diff --git a/dbus/dbus-file-win.c b/dbus/dbus-file-win.c index 06a8ea1..0dbe11e 100644 --- a/dbus/dbus-file-win.c +++ b/dbus/dbus-file-win.c @@ -251,9 +251,9 @@ _dbus_string_save_to_file (const DBusString *str, } #define N_TMP_FILENAME_RANDOM_BYTES 8 - if (!_dbus_generate_random_ascii (&tmp_filename, N_TMP_FILENAME_RANDOM_BYTES)) + if (!_dbus_generate_random_ascii (&tmp_filename, N_TMP_FILENAME_RANDOM_BYTES, + error)) { - dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL); _dbus_string_free (&tmp_filename); return FALSE; } diff --git a/dbus/dbus-internals.c b/dbus/dbus-internals.c index d988d59..2b433ef 100644 --- a/dbus/dbus-internals.c +++ b/dbus/dbus-internals.c @@ -651,8 +651,11 @@ dbus_bool_t _dbus_generate_uuid (DBusGUID *uuid, DBusError *error) { + DBusError rand_error; long now; + dbus_error_init (&rand_error); + /* don't use monotonic time because the UUID may be saved to disk, e.g. * it may persist across reboots */ @@ -660,7 +663,16 @@ _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); + if (!_dbus_generate_random_bytes_buffer (uuid->as_bytes, + DBUS_UUID_LENGTH_BYTES - 4, + &rand_error)) + { + dbus_set_error (error, rand_error.name, + "Failed to generate UUID: %s", rand_error.message); + dbus_error_free (&rand_error); + return FALSE; + } + return TRUE; } diff --git a/dbus/dbus-keyring.c b/dbus/dbus-keyring.c index f0c64de..bb7e4f8 100644 --- a/dbus/dbus-keyring.c +++ b/dbus/dbus-keyring.c @@ -304,11 +304,8 @@ add_new_key (DBusKey **keys_p, /* Generate an integer ID and then the actual key. */ retry: - if (!_dbus_generate_random_bytes (&bytes, 4)) - { - dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL); - goto out; - } + if (!_dbus_generate_random_bytes (&bytes, 4, error)) + goto out; s = (const unsigned char*) _dbus_string_get_const_data (&bytes); @@ -329,9 +326,8 @@ add_new_key (DBusKey **keys_p, #define KEY_LENGTH_BYTES 24 _dbus_string_set_length (&bytes, 0); - if (!_dbus_generate_random_bytes (&bytes, KEY_LENGTH_BYTES)) + if (!_dbus_generate_random_bytes (&bytes, KEY_LENGTH_BYTES, error)) { - dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL); goto out; } diff --git a/dbus/dbus-nonce.c b/dbus/dbus-nonce.c index c46f15a..3ba81f9 100644 --- a/dbus/dbus-nonce.c +++ b/dbus/dbus-nonce.c @@ -186,9 +186,8 @@ generate_and_write_nonce (const DBusString *filename, DBusError *error) return FALSE; } - if (!_dbus_generate_random_bytes (&nonce, 16)) + if (!_dbus_generate_random_bytes (&nonce, 16, error)) { - dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL); _dbus_string_free (&nonce); return FALSE; } @@ -270,9 +269,8 @@ do_noncefile_create (DBusNonceFile *noncefile, goto on_error; } - if (!_dbus_generate_random_ascii (&randomStr, 8)) + if (!_dbus_generate_random_ascii (&randomStr, 8, error)) { - dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL); goto on_error; } diff --git a/dbus/dbus-server-unix.c b/dbus/dbus-server-unix.c index f96a3d4..eac2291 100644 --- a/dbus/dbus-server-unix.c +++ b/dbus/dbus-server-unix.c @@ -152,10 +152,22 @@ _dbus_server_listen_platform_specific (DBusAddressEntry *entry, return DBUS_SERVER_LISTEN_DID_NOT_CONNECT; } - if (!_dbus_string_append (&filename, - "dbus-") || - !_dbus_generate_random_ascii (&filename, 10) || - !_dbus_string_append (&full_path, tmpdir) || + if (!_dbus_string_append (&filename, "dbus-")) + { + _dbus_string_free (&full_path); + _dbus_string_free (&filename); + dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL); + return DBUS_SERVER_LISTEN_DID_NOT_CONNECT; + } + + if (!_dbus_generate_random_ascii (&filename, 10, error)) + { + _dbus_string_free (&full_path); + _dbus_string_free (&filename); + return DBUS_SERVER_LISTEN_DID_NOT_CONNECT; + } + + if (!_dbus_string_append (&full_path, tmpdir) || !_dbus_concat_dir_and_file (&full_path, &filename)) { _dbus_string_free (&full_path); diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c index e8178c8..9c7a533 100644 --- a/dbus/dbus-sysdeps-unix.c +++ b/dbus/dbus-sysdeps-unix.c @@ -3002,61 +3002,54 @@ _dbus_sleep_milliseconds (int milliseconds) #endif } -static dbus_bool_t -_dbus_generate_pseudorandom_bytes (DBusString *str, - int n_bytes) -{ - int old_len; - char *p; - - old_len = _dbus_string_get_length (str); - - if (!_dbus_string_lengthen (str, n_bytes)) - return FALSE; - - p = _dbus_string_get_data_len (str, old_len, n_bytes); - - _dbus_generate_pseudorandom_bytes_buffer (p, n_bytes); - - return TRUE; -} - /** - * Generates the given number of random bytes, + * Generates the given number of securely random bytes, * using the best mechanism we can come up with. * * @param str the string * @param n_bytes the number of random bytes to append to string - * @returns #TRUE on success, #FALSE if no memory + * @returns #TRUE on success, #FALSE on error */ dbus_bool_t _dbus_generate_random_bytes (DBusString *str, - int n_bytes) + int n_bytes, + DBusError *error) { int old_len; int fd; - - /* FALSE return means "no memory", if it could - * mean something else then we'd need to return - * a DBusError. So we always fall back to pseudorandom - * if the I/O fails. - */ + int result; old_len = _dbus_string_get_length (str); fd = -1; /* note, urandom on linux will fall back to pseudorandom */ fd = open ("/dev/urandom", O_RDONLY); + if (fd < 0) - return _dbus_generate_pseudorandom_bytes (str, n_bytes); + { + dbus_set_error (error, _dbus_error_from_errno (errno), + "Could not open /dev/urandom: %s", + _dbus_strerror (errno)); + return FALSE; + } _dbus_verbose ("/dev/urandom fd %d opened\n", fd); - if (_dbus_read (fd, str, n_bytes) != n_bytes) + result = _dbus_read (fd, str, n_bytes); + + if (result != n_bytes) { + if (result < 0) + dbus_set_error (error, _dbus_error_from_errno (errno), + "Could not read /dev/urandom: %s", + _dbus_strerror (errno)); + else + dbus_set_error (error, DBUS_ERROR_IO_ERROR, + "Short read from /dev/urandom"); + _dbus_close (fd, NULL); _dbus_string_set_length (str, old_len); - return _dbus_generate_pseudorandom_bytes (str, n_bytes); + return FALSE; } _dbus_verbose ("Read %d bytes from /dev/urandom\n", diff --git a/dbus/dbus-sysdeps-win.c b/dbus/dbus-sysdeps-win.c index 6e59c5e..f21c4ab 100644 --- a/dbus/dbus-sysdeps-win.c +++ b/dbus/dbus-sysdeps-win.c @@ -2262,7 +2262,8 @@ _dbus_create_directory (const DBusString *filename, */ dbus_bool_t _dbus_generate_random_bytes (DBusString *str, - int n_bytes) + int n_bytes, + DBusError *error) { int old_len; char *p; @@ -2271,15 +2272,22 @@ _dbus_generate_random_bytes (DBusString *str, old_len = _dbus_string_get_length (str); if (!_dbus_string_lengthen (str, n_bytes)) - return FALSE; + { + _DBUS_SET_OOM (error); + return FALSE; + } p = _dbus_string_get_data_len (str, old_len, n_bytes); if (!CryptAcquireContext (&hprov, NULL, NULL, PROV_RSA_FULL, CRYPT_VERIFYCONTEXT)) - return FALSE; + { + _DBUS_SET_OOM (error); + return FALSE; + } if (!CryptGenRandom (hprov, n_bytes, p)) { + _DBUS_SET_OOM (error); CryptReleaseContext (hprov, 0); return FALSE; } diff --git a/dbus/dbus-sysdeps.c b/dbus/dbus-sysdeps.c index 9979210..cfe53c7 100644 --- a/dbus/dbus-sysdeps.c +++ b/dbus/dbus-sysdeps.c @@ -504,63 +504,35 @@ _dbus_string_parse_uint (const DBusString *str, * @{ */ -void -_dbus_generate_pseudorandom_bytes_buffer (char *buffer, - int n_bytes) -{ - long tv_usec; - int i; - - /* fall back to pseudorandom */ - _dbus_verbose ("Falling back to pseudorandom for %d bytes\n", - n_bytes); - - _dbus_get_real_time (NULL, &tv_usec); - srand (tv_usec); - - i = 0; - while (i < n_bytes) - { - double r; - unsigned int b; - - r = rand (); - b = (r / (double) RAND_MAX) * 255.0; - - buffer[i] = b; - - ++i; - } -} - /** * Fills n_bytes of the given buffer with random bytes. * * @param buffer an allocated buffer * @param n_bytes the number of bytes in buffer to write to */ -void -_dbus_generate_random_bytes_buffer (char *buffer, - int n_bytes) +dbus_bool_t +_dbus_generate_random_bytes_buffer (char *buffer, + int n_bytes, + DBusError *error) { DBusString str; if (!_dbus_string_init (&str)) { - _dbus_generate_pseudorandom_bytes_buffer (buffer, n_bytes); - return; + _DBUS_SET_OOM (error); + return FALSE; } - if (!_dbus_generate_random_bytes (&str, n_bytes)) + if (!_dbus_generate_random_bytes (&str, n_bytes, error)) { _dbus_string_free (&str); - _dbus_generate_pseudorandom_bytes_buffer (buffer, n_bytes); - return; + return FALSE; } _dbus_string_copy_to_buffer (&str, buffer, n_bytes); _dbus_string_free (&str); + return TRUE; } /** @@ -573,14 +545,15 @@ _dbus_generate_random_bytes_buffer (char *buffer, */ dbus_bool_t _dbus_generate_random_ascii (DBusString *str, - int n_bytes) + int n_bytes, + DBusError *error) { static const char letters[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789abcdefghijklmnopqrstuvwxyz"; int i; int len; - if (!_dbus_generate_random_bytes (str, n_bytes)) + if (!_dbus_generate_random_bytes (str, n_bytes, error)) return FALSE; len = _dbus_string_get_length (str); diff --git a/dbus/dbus-sysdeps.h b/dbus/dbus-sysdeps.h index bbbf708..98e9a1d 100644 --- a/dbus/dbus-sysdeps.h +++ b/dbus/dbus-sysdeps.h @@ -383,15 +383,17 @@ const char* _dbus_get_tmpdir (void); /** * Random numbers */ -void _dbus_generate_pseudorandom_bytes_buffer (char *buffer, - int n_bytes); -void _dbus_generate_random_bytes_buffer (char *buffer, - int n_bytes); +_DBUS_GNUC_WARN_UNUSED_RESULT +dbus_bool_t _dbus_generate_random_bytes_buffer (char *buffer, + int n_bytes, + DBusError *error); dbus_bool_t _dbus_generate_random_bytes (DBusString *str, - int n_bytes); + int n_bytes, + DBusError *error); DBUS_PRIVATE_EXPORT dbus_bool_t _dbus_generate_random_ascii (DBusString *str, - int n_bytes); + int n_bytes, + DBusError *error); DBUS_PRIVATE_EXPORT const char* _dbus_error_from_errno (int error_number); -- 2.1.4