From de0fae3d4958c16ac01c295b20027e5bcff29d1e Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 5 May 2020 15:48:30 +0100 Subject: [PATCH 1/6] tests: Fix a memory leak in gdbus-server-auth Signed-off-by: Philip Withnall --- gio/tests/gdbus-server-auth.c | 1 + 1 file changed, 1 insertion(+) diff --git a/gio/tests/gdbus-server-auth.c b/gio/tests/gdbus-server-auth.c index d8b361fc6..e782bd3d7 100644 --- a/gio/tests/gdbus-server-auth.c +++ b/gio/tests/gdbus-server-auth.c @@ -152,6 +152,7 @@ whoami_filter_cb (GDBusConnection *connection, g_dbus_connection_send_message (connection, reply, G_DBUS_SEND_MESSAGE_FLAGS_NONE, NULL, NULL); + g_object_unref (reply); /* handled */ g_object_unref (message); From 1a35d27f1b3c2bec568ecfc9b54d863318e9893b Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 5 May 2020 15:02:07 +0100 Subject: [PATCH 2/6] tests: Isolate directory access for gdbus-server-auth test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When multiple tests were run in parallel, this would race on its access to `~/.dbus-keyrings` to authenticate with the D-Bus server, since the keyring directory was not appropriately sandboxed to the unit test. Use `G_TEST_OPTION_ISOLATE_DIRS` to automatically isolate each unit test’s directory usage. Signed-off-by: Philip Withnall Fixes: #1954 --- gio/tests/gdbus-server-auth.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/gio/tests/gdbus-server-auth.c b/gio/tests/gdbus-server-auth.c index e782bd3d7..2554ad6ab 100644 --- a/gio/tests/gdbus-server-auth.c +++ b/gio/tests/gdbus-server-auth.c @@ -398,6 +398,14 @@ do_test_server_auth (InteropFlags flags) LibdbusCall libdbus_call = { DBUS_ERROR_INIT, NULL, NULL, NULL }; GTask *task; + /* The test suite uses %G_TEST_OPTION_ISOLATE_DIRS, which sets + * `HOME=/dev/null` and leaves g_get_home_dir() pointing to the per-test + * temp home directory. Unfortunately, libdbus doesn’t allow the home dir + * to be overridden except using the environment, so copy the per-test + * temp home directory back there so that libdbus uses the same + * `$HOME/.dbus-keyrings` path as GLib. This is not thread-safe. */ + g_setenv ("HOME", g_get_home_dir (), TRUE); + libdbus_call.conn = dbus_connection_open_private (connectable_address, &libdbus_call.error); g_assert_cmpstr (libdbus_call.error.name, ==, NULL); @@ -516,10 +524,7 @@ int main (int argc, char *argv[]) { - /* FIXME: Add debug for https://gitlab.gnome.org/GNOME/glib/issues/1954 */ - g_setenv ("G_DBUS_DEBUG", "all", TRUE); - - g_test_init (&argc, &argv, NULL); + g_test_init (&argc, &argv, G_TEST_OPTION_ISOLATE_DIRS, NULL); g_test_add_func ("/gdbus/server-auth", test_server_auth); g_test_add_func ("/gdbus/server-auth/abstract", test_server_auth_abstract); From d20664c6d4aef92510ffab60fc55cec3249a3e66 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 5 May 2020 15:12:11 +0100 Subject: [PATCH 3/6] gdbusauthmechanismsha1: Fix return type in precondition guards Signed-off-by: Philip Withnall --- gio/gdbusauthmechanismsha1.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gio/gdbusauthmechanismsha1.c b/gio/gdbusauthmechanismsha1.c index 2754d3c2b..7305f8df8 100644 --- a/gio/gdbusauthmechanismsha1.c +++ b/gio/gdbusauthmechanismsha1.c @@ -462,8 +462,8 @@ keyring_acquire_lock (const gchar *path, #endif int errsv; - g_return_val_if_fail (path != NULL, FALSE); - g_return_val_if_fail (error == NULL || *error == NULL, FALSE); + 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); From a0689d21e4a0429d81119e99c586e1a77c4d5e2b Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 5 May 2020 15:01:07 +0100 Subject: [PATCH 4/6] gdbusauthmechanismsha1: Fix keyring lock file permissions Why should it have the sticky bit set? dbus.git sets permissions 0600; so should GLib. Signed-off-by: Philip Withnall --- gio/gdbusauthmechanismsha1.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gio/gdbusauthmechanismsha1.c b/gio/gdbusauthmechanismsha1.c index 7305f8df8..0d58b1321 100644 --- a/gio/gdbusauthmechanismsha1.c +++ b/gio/gdbusauthmechanismsha1.c @@ -518,7 +518,7 @@ keyring_acquire_lock (const gchar *path, #else 0, #endif - 0700); + 0600); errsv = errno; if (ret == -1) { From b4664e237a331f5b65e9b25397c52ec5530e8b43 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 5 May 2020 15:42:24 +0100 Subject: [PATCH 5/6] gdbusauthmechanismsha1: Fix race in keyring_acquire_lock() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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; } From f3b6700256b6ba2ece3b161cca1641c0487820c8 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 5 May 2020 15:40:46 +0100 Subject: [PATCH 6/6] gdbusauthmechanismsha1: Reduce syscalls from ensure_keyring_directory() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There’s no need to call `access()` and then `stat()` on the keyring directory to check that it exists, is a directory, and has the right permissions. Just call `stat()`. This eliminates one potential TOCTTOU race in this code. Signed-off-by: Philip Withnall Helps: #1954 --- gio/gdbusauthmechanismsha1.c | 85 +++++++++++++++++++----------------- 1 file changed, 46 insertions(+), 39 deletions(-) diff --git a/gio/gdbusauthmechanismsha1.c b/gio/gdbusauthmechanismsha1.c index 9385ed155..5e3e93d13 100644 --- a/gio/gdbusauthmechanismsha1.c +++ b/gio/gdbusauthmechanismsha1.c @@ -234,6 +234,9 @@ ensure_keyring_directory (GError **error) { gchar *path; const gchar *e; +#ifdef G_OS_UNIX + struct stat statbuf; +#endif g_return_val_if_fail (error == NULL || *error == NULL, NULL); @@ -249,48 +252,54 @@ ensure_keyring_directory (GError **error) NULL); } +#ifdef G_OS_UNIX + if (stat (path, &statbuf) != 0) + { + int errsv = errno; + + if (errsv != ENOENT) + { + g_set_error (error, + G_IO_ERROR, + g_io_error_from_errno (errsv), + _("Error when getting information for directory “%s”: %s"), + path, + g_strerror (errsv)); + g_clear_pointer (&path, g_free); + return NULL; + } + } + else if (S_ISDIR (statbuf.st_mode)) + { + if (g_getenv ("G_DBUS_COOKIE_SHA1_KEYRING_DIR_IGNORE_PERMISSION") == NULL && + (statbuf.st_mode & 0777) != 0700) + { + g_set_error (error, + G_IO_ERROR, + G_IO_ERROR_FAILED, + _("Permissions on directory “%s” are malformed. Expected mode 0700, got 0%o"), + path, + (guint) (statbuf.st_mode & 0777)); + g_clear_pointer (&path, g_free); + return NULL; + } + + return g_steal_pointer (&path); + } +#else /* if !G_OS_UNIX */ + /* On non-Unix platforms, check that it exists as a directory, but don’t do + * permissions checks at the moment. */ if (g_file_test (path, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR)) { - if (g_getenv ("G_DBUS_COOKIE_SHA1_KEYRING_DIR_IGNORE_PERMISSION") == NULL) - { -#ifdef G_OS_UNIX - struct stat statbuf; - if (stat (path, &statbuf) != 0) - { - int errsv = errno; - g_set_error (error, - G_IO_ERROR, - g_io_error_from_errno (errsv), - _("Error when getting information for directory “%s”: %s"), - path, - g_strerror (errsv)); - g_free (path); - path = NULL; - goto out; - } - if ((statbuf.st_mode & 0777) != 0700) - { - g_set_error (error, - G_IO_ERROR, - G_IO_ERROR_FAILED, - _("Permissions on directory “%s” are malformed. Expected mode 0700, got 0%o"), - path, - (guint) (statbuf.st_mode & 0777)); - g_free (path); - path = NULL; - goto out; - } -#else #ifdef __GNUC__ #pragma GCC diagnostic push #pragma GCC diagnostic warning "-Wcpp" #warning Please implement permission checking on this non-UNIX platform #pragma GCC diagnostic pop -#endif -#endif - } - goto out; +#endif /* __GNUC__ */ + return g_steal_pointer (&path); } +#endif /* if !G_OS_UNIX */ if (g_mkdir_with_parents (path, 0700) != 0) { @@ -301,13 +310,11 @@ ensure_keyring_directory (GError **error) _("Error creating directory “%s”: %s"), path, g_strerror (errsv)); - g_free (path); - path = NULL; - goto out; + g_clear_pointer (&path, g_free); + return NULL; } -out: - return path; + return g_steal_pointer (&path); } /* ---------------------------------------------------------------------------------------------------- */