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
Reviewed-by: Ralf Habacker <ralf.habacker@freenet.de>
[smcv: document @error]
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>

12 files changed, 146 insertions, 119 deletions

diff --git a/bus/activation.c b/bus/activation.c
index 390b8362..679a40eb 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 1503d5f1..f2227875 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 19759336..830d3fe4 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 06a8ea1c..0dbe11ec 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 24e070d1..30a5fa73 100644
--- a/dbus/dbus-internals.c
+++ b/dbus/dbus-internals.c
@@ -653,8 +653,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
    */
@@ -662,7 +665,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 f0c64de3..bb7e4f8d 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 e5685e16..3c0f6f37 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;
     }
@@ -272,9 +271,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 5474177a..92664a8d 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 c9dbe7ba..dc0418cb 100644
--- a/dbus/dbus-sysdeps-unix.c
+++ b/dbus/dbus-sysdeps-unix.c
@@ -3003,61 +3003,55 @@ _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
+ * @param error location to store reason for failure
+ * @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 143470ab..b3b459f8 100644
--- a/dbus/dbus-sysdeps-win.c
+++ b/dbus/dbus-sysdeps-win.c
@@ -2253,11 +2253,13 @@ _dbus_create_directory (const DBusString *filename,
  *
  * @param str the string
  * @param n_bytes the number of random bytes to append to string
- * @returns #TRUE on success, #FALSE if no memory
+ * @param error location to store reason for failure
+ * @returns #TRUE on success
  */
 dbus_bool_t
 _dbus_generate_random_bytes (DBusString *str,
-                             int         n_bytes)
+                             int         n_bytes,
+                             DBusError  *error)
 {
   int old_len;
   char *p;
@@ -2266,15 +2268,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 99792100..8b986d58 100644
--- a/dbus/dbus-sysdeps.c
+++ b/dbus/dbus-sysdeps.c
@@ -504,63 +504,37 @@ _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
+ * @param error location to store reason for failure
+ * @returns #TRUE on success
  */
-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;
 }
 
 /**
@@ -569,18 +543,20 @@ _dbus_generate_random_bytes_buffer (char *buffer,
  *
  * @param str the string
  * @param n_bytes the number of random ASCII bytes to append to string
+ * @param error location to store reason for failure
  * @returns #TRUE on success, #FALSE if no memory or other failure
  */
 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 f6a2948e..7043a452 100644
--- a/dbus/dbus-sysdeps.h
+++ b/dbus/dbus-sysdeps.h
@@ -476,15 +476,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);