[glib/glib-2-64: 5/7] gdbusauthmechanismsha1: Fix race in keyring_acquire_lock()

[Simon McVittie <smcv src gnome org>]

commit b4664e237a331f5b65e9b25397c52ec5530e8b43
Author: Philip Withnall <withnall endlessm com>
Date:   Tue May 5 15:42:24 2020 +0100

    gdbusauthmechanismsha1: Fix race in keyring_acquire_lock()
    
    There was a time-of-check-to-time-of-use (TOCTTOU) race in the keyring
    lock code, where it would check the existence of the lock file using
    `access()`, then proceed to call `open(O_CREAT | O_EXCL)` to try and
    create the lock file once `access()` showed that it didn’t exist.
    
    The problem is that, because this is happening in a shared directory
    (`~/.dbus-keyrings`), another process could quite legitimately create
    the lock file in the meantime.
    
    Instead, unconditionally call `open()` and ignore errors from it (which
    will be returned if the lock file already exists) until it succeeds (or
    the code times out).
    
    This eliminates the TOCTTOU race, and simplifies the timeout behaviour
    so there aren’t two loops (check for existence, try to create)
    happening. It brings this code in line with what dbus.git does (see
    `_dbus_keyring_lock()`).
    
    Signed-off-by: Philip Withnall <withnall endlessm com>
    
    Fixes: #1954

 gio/gdbusauthmechanismsha1.c | 116 ++++++++++++++++++++++---------------------
 1 file changed, 60 insertions(+), 56 deletions(-)
---
diff --git a/gio/gdbusauthmechanismsha1.c b/gio/gdbusauthmechanismsha1.c
index 0d58b1321..9385ed155 100644
--- a/gio/gdbusauthmechanismsha1.c
+++ b/gio/gdbusauthmechanismsha1.c
@@ -450,25 +450,52 @@ _log (const gchar *message,
   g_free (s);
 }
 
+/* Returns FD for lock file, if it was created exclusively (didn't exist already,
+ * and was created successfully) */
+static gint
+create_lock_exclusive (const gchar  *lock_path,
+                       GError      **error)
+{
+  int errsv;
+  gint ret;
+
+  ret = g_open (lock_path, O_CREAT | O_EXCL, 0600);
+  errsv = errno;
+  if (ret < 0)
+    {
+      g_set_error (error,
+                   G_IO_ERROR,
+                   g_io_error_from_errno (errsv),
+                   _("Error creating lock file “%s”: %s"),
+                   lock_path,
+                   g_strerror (errsv));
+      return -1;
+    }
+
+  return ret;
+}
+
 static gint
 keyring_acquire_lock (const gchar  *path,
                       GError      **error)
 {
-  gchar *lock;
+  gchar *lock = NULL;
   gint ret;
   guint num_tries;
-#ifdef EEXISTS
-  guint num_create_tries;
-#endif
   int errsv;
 
+  /* Total possible sleep period = max_tries * timeout_usec = 0.5s */
+  const guint max_tries = 50;
+  const guint timeout_usec = 1000 * 10;
+
   g_return_val_if_fail (path != NULL, -1);
   g_return_val_if_fail (error == NULL || *error == NULL, -1);
 
   ret = -1;
-  lock = g_strdup_printf ("%s.lock", path);
+  lock = g_strconcat (path, ".lock", NULL);
 
   /* This is what the D-Bus spec says
+   * (https://dbus.freedesktop.org/doc/dbus-specification.html#auth-mechanisms-sha)
    *
    *  Create a lockfile name by appending ".lock" to the name of the
    *  cookie file. The server should attempt to create this file using
@@ -481,66 +508,43 @@ keyring_acquire_lock (const gchar  *path,
    *         real locking implementations are still flaky on network filesystems
    */
 
-#ifdef EEXISTS
-  num_create_tries = 0;
- again:
-#endif
-  num_tries = 0;
-  while (g_file_test (lock, G_FILE_TEST_EXISTS))
+  for (num_tries = 0; num_tries < max_tries; num_tries++)
     {
+      /* Ignore the error until the final call. */
+      ret = create_lock_exclusive (lock, NULL);
+      if (ret >= 0)
+        break;
+
       /* sleep 10ms, then try again */
-      g_usleep (1000*10);
-      num_tries++;
-      if (num_tries == 50)
-        {
-          /* ok, we slept 50*10ms = 0.5 seconds. Conclude that the lock file must be
-           * stale (nuke the it from orbit)
-           */
-          if (g_unlink (lock) != 0)
-            {
-              errsv = errno;
-              g_set_error (error,
-                           G_IO_ERROR,
-                           g_io_error_from_errno (errsv),
-                           _("Error deleting stale lock file “%s”: %s"),
-                           lock,
-                           g_strerror (errsv));
-              goto out;
-            }
-          _log ("Deleted stale lock file '%s'", lock);
-          break;
-        }
+      g_usleep (timeout_usec);
     }
 
-  ret = g_open (lock, O_CREAT |
-#ifdef O_EXCL
-                O_EXCL,
-#else
-                0,
-#endif
-                0600);
-  errsv = errno;
-  if (ret == -1)
+  if (num_tries == max_tries)
     {
-#ifdef EEXISTS
-      /* EEXIST: pathname already exists and O_CREAT and O_EXCL were used. */
-      if (errsv == EEXISTS)
+      /* ok, we slept 50*10ms = 0.5 seconds. Conclude that the lock file must be
+       * stale (nuke it from orbit)
+       */
+      if (g_unlink (lock) != 0)
         {
-          num_create_tries++;
-          if (num_create_tries < 5)
-            goto again;
+          errsv = errno;
+          g_set_error (error,
+                       G_IO_ERROR,
+                       g_io_error_from_errno (errsv),
+                       _("Error deleting stale lock file “%s”: %s"),
+                       lock,
+                       g_strerror (errsv));
+          goto out;
         }
-#endif
-      g_set_error (error,
-                   G_IO_ERROR,
-                   g_io_error_from_errno (errsv),
-                   _("Error creating lock file “%s”: %s"),
-                   lock,
-                   g_strerror (errsv));
-      goto out;
+
+      _log ("Deleted stale lock file '%s'", lock);
+
+      /* Try one last time to create it, now that we've deleted the stale one */
+      ret = create_lock_exclusive (lock, error);
+      if (ret < 0)
+        goto out;
     }
 
- out:
+out:
   g_free (lock);
   return ret;
 }