Attachment 129666 Details for Bug 99828 – Change _dbus_create_directory to fail for existing directories

[patch] Change _dbus_create_directory to fail for existing directories

0001-Change-_dbus_create_directory-to-fail-for-existing-d.patch (text/plain), 6.22 KB, created by Simon McVittie on 2017-02-16 13:02:13 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2017-02-16 13:02:13 UTC

Size: 6.22 KB

patch

obsolete

>From f2722062889b521ca40436b62044922c1d11d450 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Wed, 15 Feb 2017 16:32:04 +0000
>Subject: [PATCH 1/4] 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.
>
>Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
>---
> dbus/dbus-keyring.c      |  2 +-
> dbus/dbus-sysdeps-unix.c | 31 ++++++++++++++++++++++++++++++-
> dbus/dbus-sysdeps-win.c  | 31 ++++++++++++++++++++++++++++++-
> dbus/dbus-sysdeps.h      |  3 +++
> 4 files changed, 64 insertions(+), 3 deletions(-)
>
>diff --git a/dbus/dbus-keyring.c b/dbus/dbus-keyring.c
>index 6512a1d5..d9adaa6a 100644
>--- 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 377e9e27..39c13360 100644
>--- a/dbus/dbus-sysdeps-unix.c
>+++ b/dbus/dbus-sysdeps-unix.c
>@@ -2955,7 +2955,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;
>@@ -2979,6 +2979,35 @@ _dbus_create_directory (const DBusString *filename,
> }
> 
> /**
>+ * 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.
>  *
>  * @todo it might be cute to collapse multiple '/' such as "foo//"
>diff --git a/dbus/dbus-sysdeps-win.c b/dbus/dbus-sysdeps-win.c
>index 77c5b74e..e321168b 100644
>--- a/dbus/dbus-sysdeps-win.c
>+++ b/dbus/dbus-sysdeps-win.c
>@@ -2228,6 +2228,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.
>  *
>@@ -2236,7 +2265,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 2ef4bf2c..543ce57f 100644
>--- a/dbus/dbus-sysdeps.h
>+++ b/dbus/dbus-sysdeps.h
>@@ -412,6 +412,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);
> 
>-- 
>2.11.0
>

Actions: View | Splinter Review

Attachments on bug 99828: 129643 | 129652 | 129653 | 129654 | 129655 | 129666 | 129667

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.