Attachment 115718 Details for Bug 90414 – Fail to generate random bytes instead of falling back to rand()

[patch] Fail to generate random bytes instead of falling back to rand()

0005-Fail-to-generate-random-bytes-instead-of-falling-bac.patch (text/plain), 17.91 KB, created by Simon McVittie on 2015-05-12 11:53:09 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2015-05-12 11:53:09 UTC

Size: 17.91 KB

patch

obsolete

>From 545570a91ecf6bb38c1d1ab7c80165efa60523e1 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>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
>

Actions: View | Splinter Review

Attachments on bug 90414: 115714 | 115715 | 115716 | 115717 | 115718

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.