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

10 files changed, 96 insertions, 43 deletions

diff --git a/bus/bus.c b/bus/bus.c
index 68de1b22..1aa893b6 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 27429e88..81b3a838 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 c7f81e58..24e070d1 100644
--- a/dbus/dbus-internals.c
+++ b/dbus/dbus-internals.c
@@ -646,9 +646,12 @@ _dbus_string_array_contains (const char **array,
  * there's some text about it in the spec that should also change.
  *
  * @param uuid the uuid to initialize
+ * @param error location to store reason for failure
+ * @returns #TRUE on success
  */
-void
-_dbus_generate_uuid (DBusGUID *uuid)
+dbus_bool_t
+_dbus_generate_uuid (DBusGUID  *uuid,
+                     DBusError *error)
 {
   long now;
 
@@ -660,6 +663,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 +845,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);
     }
 }
@@ -858,22 +865,27 @@ static DBusGUID machine_uuid;
  * machine is reconfigured or its hardware is modified.
  * 
  * @param uuid_str string to append hex-encoded machine uuid to
- * @returns #FALSE if no memory
+ * @param error location to store reason for failure
+ * @returns #TRUE if successful
  */
 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 +894,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 225a9b66..3eb8749f 100644
--- a/dbus/dbus-internals.h
+++ b/dbus/dbus-internals.h
@@ -389,8 +389,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);
@@ -403,7 +404,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 6ca30f24..a2da562a 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 42891bde..9af906fa 100644
--- a/dbus/dbus-server.c
+++ b/dbus/dbus-server.c
@@ -137,7 +137,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;
@@ -167,6 +168,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 1d685897..c9dbe7ba 100644
--- a/dbus/dbus-sysdeps-unix.c
+++ b/dbus/dbus-sysdeps-unix.c
@@ -3742,9 +3742,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;
     }
 
@@ -3844,7 +3843,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 6d7c0aec..b4041633 100644
--- a/dbus/dbus-uuidgen.c
+++ b/dbus/dbus-uuidgen.c
@@ -111,20 +111,20 @@ 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
+ * @param error location to store reason for failure
+ * @returns #TRUE on success
  */
 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 9d12f2bc..5e4a948c 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 c8ba1cf7..03ce5536 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))