Change _dbus_create_directory to fail for existing directories

If we don't trap EEXIST and its Windows equivalent, we are unable to
detect the situation where we create an ostensibly unique
subdirectory in a shared /tmp, but an attacker has already created it.
This affects dbus-nonce (the nonce-tcp transport) and the activation
reload test.

Add a new _dbus_ensure_directory() for the one case where we want it to
succeed even on EEXIST: the DBUS_COOKIE_SHA1 keyring, which we know
we are creating in our own trusted "official" $HOME. In the new
transient service support on Bug #99825, ensure_owned_directory()
would need the same treatment.

We are not treating this as a serious security problem, because the
nonce-tcp transport is rarely enabled on Unix and there are multiple
mitigations.

The nonce-tcp transport creates a new unique file with O_EXCL and 0600
(private to user) permissions, then overwrites the requested filename
via atomic-overwrite, so the worst that could happen there is that an
attacker could place a symbolic link matching the name of a directory
we are going to create, causing a dbus-daemon configured for nonce-tcp
to traverse the symlink and atomically overwrite a file named "nonce"
in a directory of the attacker's choice, with new random contents that
are not known to the attacker. This seems unlikely to be exploitable
for anything worse than denial of service in practice. In mainline
Linux since 3.6, this attack is also defeated by the
fs.protected_symlinks sysctl, which many distributions enable by default.

The activation reload test suffers from a classic symlink attack
due to time-of-check/time-of-use errors in its implementation, but as
part of the developer-only "embedded tests" that are only intended
to be run on a trusted machine, it is not treated as security-sensitive.
That code path will be fixed in a subsequent commit.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=99828
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: Philip Withnall <withnall@endlessm.com>

diff --git a/dbus/dbus-keyring.c b/dbus/dbus-keyring.c
index bb7e4f8d04f495abc4a633f315c14ff6a3bab5de..68b05794196ca7c9b73fda8c5ada3fe888b866c7 100644 (file)
--- a/dbus/dbus-keyring.c
+++ b/dbus/dbus-keyring.c
@@ -807,7 +807,7 @@ _dbus_keyring_new_for_credentials (DBusCredentials  *credentials,
    * unless someone else manages to create it
    */
   dbus_error_init (&tmp_error);
-  if (!_dbus_create_directory (&keyring->directory,
+  if (!_dbus_ensure_directory (&keyring->directory,
                                &tmp_error))
     {
       _dbus_verbose ("Creating keyring directory: %s\n",

diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c
index ed776a7044663ca1272d01388802a8514babb30a..9d914f8c375ea400bef61c7a5241d1081c5f398a 100644 (file)
--- a/dbus/dbus-sysdeps-unix.c
+++ b/dbus/dbus-sysdeps-unix.c
@@ -2953,7 +2953,7 @@ _dbus_get_real_time (long *tv_sec,
  * @returns #TRUE on success
  */
 dbus_bool_t
-_dbus_create_directory (const DBusString *filename,
+_dbus_ensure_directory (const DBusString *filename,
                         DBusError        *error)
 {
   const char *filename_c;
@@ -2976,6 +2976,35 @@ _dbus_create_directory (const DBusString *filename,
     return TRUE;
 }
 
+/**
+ * Creates a directory. Unlike _dbus_ensure_directory(), this only succeeds
+ * if the directory is genuinely newly-created.
+ *
+ * @param filename directory filename
+ * @param error initialized error object
+ * @returns #TRUE on success
+ */
+dbus_bool_t
+_dbus_create_directory (const DBusString *filename,
+                        DBusError        *error)
+{
+  const char *filename_c;
+
+  _DBUS_ASSERT_ERROR_IS_CLEAR (error);
+
+  filename_c = _dbus_string_get_const_data (filename);
+
+  if (mkdir (filename_c, 0700) < 0)
+    {
+      dbus_set_error (error, DBUS_ERROR_FAILED,
+                      "Failed to create directory %s: %s\n",
+                      filename_c, _dbus_strerror (errno));
+      return FALSE;
+    }
+  else
+    return TRUE;
+}
+
 /**
  * Appends the given filename to the given directory.
  *

diff --git a/dbus/dbus-sysdeps-win.c b/dbus/dbus-sysdeps-win.c
index d19267e67d61dd50fe76806b1f04b5c0d2e0cf77..c5219ab78eb4461bfbaee0982af588fc4d136543 100644 (file)
--- a/dbus/dbus-sysdeps-win.c
+++ b/dbus/dbus-sysdeps-win.c
@@ -2223,6 +2223,35 @@ _dbus_disable_sigpipe (void)
 {
 }
 
+/**
+ * Creates a directory. Unlike _dbus_ensure_directory(), this only succeeds
+ * if the directory is genuinely newly-created.
+ *
+ * @param filename directory filename
+ * @param error initialized error object
+ * @returns #TRUE on success
+ */
+dbus_bool_t
+_dbus_create_directory (const DBusString *filename,
+                        DBusError        *error)
+{
+  const char *filename_c;
+
+  _DBUS_ASSERT_ERROR_IS_CLEAR (error);
+
+  filename_c = _dbus_string_get_const_data (filename);
+
+  if (!CreateDirectoryA (filename_c, NULL))
+    {
+      dbus_set_error (error, DBUS_ERROR_FAILED,
+                      "Failed to create directory %s: %s\n",
+                      filename_c, _dbus_strerror_from_errno ());
+      return FALSE;
+    }
+  else
+    return TRUE;
+}
+
 /**
  * Creates a directory; succeeds if the directory
  * is created or already existed.
@@ -2232,7 +2261,7 @@ _dbus_disable_sigpipe (void)
  * @returns #TRUE on success
  */
 dbus_bool_t
-_dbus_create_directory (const DBusString *filename,
+_dbus_ensure_directory (const DBusString *filename,
                         DBusError        *error)
 {
   const char *filename_c;

diff --git a/dbus/dbus-sysdeps.h b/dbus/dbus-sysdeps.h
index 2d152b8c44c9a5903fcd81cd2b8890603da4d469..0ee45c97e3449fc188f471ecf397a82ae615feaf 100644 (file)
--- a/dbus/dbus-sysdeps.h
+++ b/dbus/dbus-sysdeps.h
@@ -421,6 +421,9 @@ DBUS_PRIVATE_EXPORT
 dbus_bool_t    _dbus_create_directory        (const DBusString *filename,
                                               DBusError        *error);
 DBUS_PRIVATE_EXPORT
+dbus_bool_t    _dbus_ensure_directory        (const DBusString *filename,
+                                              DBusError        *error);
+DBUS_PRIVATE_EXPORT
 dbus_bool_t    _dbus_delete_directory        (const DBusString *filename,
                                              DBusError        *error);