From be51bfe915af717aa462fc7957e5a3c6dabc746b Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 15 Feb 2017 16:32:04 +0000 Subject: [PATCH] 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 Reviewed-by: Philip Withnall --- 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 bb7e4f8..68b0579 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 ed776a7..9d914f8 100644 --- 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; @@ -2977,6 +2977,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 d19267e..c5219ab 100644 --- a/dbus/dbus-sysdeps-win.c +++ b/dbus/dbus-sysdeps-win.c @@ -2224,6 +2224,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 2d152b8..0ee45c9 100644 --- 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); -- 2.7.4