From 6a3f563a4b9449b257015f6118821057239a395b Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 19 Dec 2014 18:49:33 +0000 Subject: [PATCH 1/5] Hardening: reject UpdateActivationEnvironment on non-canonical path UpdateActivationEnvironment is the one dbus-daemon API call that is obviously dangerous (it is intended for the session bus), so the default system.conf does not allow anyone to call it. It has recently come to the D-Bus maintainers' attention that some system services incorrectly install D-Bus policy rules that allow arbitrary method calls to any destination as long as they have a "safe" object path. This is not actually safe: some system services that use low-level D-Bus bindings like libdbus, including dbus-daemon itself, provide the same API on all object paths. Unauthorized calls to UpdateActivationEnvironment are probably just resource consumption rather than privilege escalation, because on the system bus, the modified environment is only used to execute a setuid wrapper that avoids LD_PRELOAD etc. via normal setuid handling, and sanitizes its own environment before executing the real service. However, it's safest to assume the worst and treat it as a potential privilege escalation. Accordingly, as a hardening measure to avoid privilege escalation on systems with these faulty services, stop allowing calls to ("/com/example/Whatever", "org.freedesktop.DBus.UpdateActivationEnvironment") and only allow ("/org/freedesktop/DBus", "org.freedesktop.DBus.UpdateActivationEnvironment"). We deliberately continue to provide read-only APIs like GetConnectionUnixUser at all object paths, for backwards compatibility. Reviewed-by: Thiago Macieira [adjusted commit message to note that this is probably only DoS -smcv] --- bus/driver.c | 35 +++++++++++++++++++++++++++++++++++ bus/driver.h | 4 ++-- 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/bus/driver.c b/bus/driver.c index e95a79d9..0b9c3ed5 100644 --- a/bus/driver.c +++ b/bus/driver.c @@ -878,6 +878,9 @@ bus_driver_handle_update_activation_environment (DBusConnection *connection, _DBUS_ASSERT_ERROR_IS_CLEAR (error); + if (!bus_driver_check_message_is_for_us (message, error)) + return FALSE; + activation = bus_connection_get_activation (connection); dbus_message_iter_init (message, &iter); @@ -1965,6 +1968,38 @@ bus_driver_handle_introspect (DBusConnection *connection, return FALSE; } +/* + * Set @error and return FALSE if the message is not directed to the + * dbus-daemon by its canonical object path. This is hardening against + * system services with poorly-written security policy files, which + * might allow sending dangerously broad equivalence classes of messages + * such as "anything with this assumed-to-be-safe object path". + * + * dbus-daemon is unusual in that it normally ignores the object path + * of incoming messages; we need to keep that behaviour for the "read" + * read-only method calls like GetConnectionUnixUser for backwards + * compatibility, but it seems safer to be more restrictive for things + * intended to be root-only or privileged-developers-only. + * + * It is possible that there are other system services with the same + * quirk as dbus-daemon. + */ +dbus_bool_t +bus_driver_check_message_is_for_us (DBusMessage *message, + DBusError *error) +{ + if (!dbus_message_has_path (message, DBUS_PATH_DBUS)) + { + dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED, + "Method '%s' is only available at the canonical object path '%s'", + dbus_message_get_member (message), DBUS_PATH_DBUS); + + return FALSE; + } + + return TRUE; +} + dbus_bool_t bus_driver_handle_message (DBusConnection *connection, BusTransaction *transaction, diff --git a/bus/driver.h b/bus/driver.h index 713b2764..201709c4 100644 --- a/bus/driver.h +++ b/bus/driver.h @@ -46,7 +46,7 @@ dbus_bool_t bus_driver_send_service_owner_changed (const char *service_name BusTransaction *transaction, DBusError *error); dbus_bool_t bus_driver_generate_introspect_string (DBusString *xml); - - +dbus_bool_t bus_driver_check_message_is_for_us (DBusMessage *message, + DBusError *error); #endif /* BUS_DRIVER_H */ From a67cb9bf1c092e9ade210cb9d894664298687f8f Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 19 Dec 2014 19:19:00 +0000 Subject: [PATCH 2/5] Hardening: only allow the uid of the dbus-daemon to call UpdateActivationEnvironment As with the previous commit, this is probably not actually privilege escalation due to the use of an activation helper that cleans up its environment, but let's be extra-careful here. Reviewed-by: Thiago Macieira [adjusted commit message -smcv] --- bus/driver.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/bus/driver.c b/bus/driver.c index 0b9c3ed5..f5d3ebe2 100644 --- a/bus/driver.c +++ b/bus/driver.c @@ -881,6 +881,41 @@ bus_driver_handle_update_activation_environment (DBusConnection *connection, if (!bus_driver_check_message_is_for_us (message, error)) return FALSE; +#ifdef DBUS_UNIX + { + /* UpdateActivationEnvironment is basically a recipe for privilege + * escalation so let's be extra-careful: do not allow the sysadmin + * to shoot themselves in the foot. */ + unsigned long uid; + + if (!dbus_connection_get_unix_user (connection, &uid)) + { + bus_context_log (bus_transaction_get_context (transaction), + DBUS_SYSTEM_LOG_SECURITY, + "rejected attempt to call UpdateActivationEnvironment by " + "unknown uid"); + dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED, + "rejected attempt to call UpdateActivationEnvironment by " + "unknown uid"); + return FALSE; + } + + /* On the system bus, we could in principle allow uid 0 to call + * UpdateActivationEnvironment; but they should know better anyway, + * and our default system.conf has always forbidden it */ + if (!_dbus_unix_user_is_process_owner (uid)) + { + bus_context_log (bus_transaction_get_context (transaction), + DBUS_SYSTEM_LOG_SECURITY, + "rejected attempt to call UpdateActivationEnvironment by uid %lu", + uid); + dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED, + "rejected attempt to call UpdateActivationEnvironment"); + return FALSE; + } + } +#endif + activation = bus_connection_get_activation (connection); dbus_message_iter_init (message, &iter); From 4daf4bdc92d73a630634272a529c2690e2348eb9 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 19 Dec 2014 19:17:14 +0000 Subject: [PATCH 3/5] Add a regression test for path-based UpdateActivationEnvironment hardening Reviewed-by: Thiago Macieira --- test/dbus-daemon.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/test/dbus-daemon.c b/test/dbus-daemon.c index 4b3b61e5..dc0f1317 100644 --- a/test/dbus-daemon.c +++ b/test/dbus-daemon.c @@ -457,6 +457,91 @@ test_creds (Fixture *f, #endif } +static void +test_canonical_path_uae (Fixture *f, + gconstpointer context) +{ + DBusMessage *m = dbus_message_new_method_call (DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, "UpdateActivationEnvironment"); + DBusPendingCall *pc; + DBusMessageIter args_iter; + DBusMessageIter arr_iter; + + if (m == NULL) + g_error ("OOM"); + + dbus_message_iter_init_append (m, &args_iter); + + /* Append an empty a{ss} (string => string dictionary). */ + if (!dbus_message_iter_open_container (&args_iter, DBUS_TYPE_ARRAY, + "{ss}", &arr_iter) || + !dbus_message_iter_close_container (&args_iter, &arr_iter)) + g_error ("OOM"); + + if (!dbus_connection_send_with_reply (f->left_conn, m, &pc, + DBUS_TIMEOUT_USE_DEFAULT) || + pc == NULL) + g_error ("OOM"); + + dbus_message_unref (m); + m = NULL; + + if (dbus_pending_call_get_completed (pc)) + pending_call_store_reply (pc, &m); + else if (!dbus_pending_call_set_notify (pc, pending_call_store_reply, + &m, NULL)) + g_error ("OOM"); + + while (m == NULL) + test_main_context_iterate (f->ctx, TRUE); + + /* it succeeds */ + g_assert_cmpint (dbus_message_get_type (m), ==, + DBUS_MESSAGE_TYPE_METHOD_RETURN); + + dbus_message_unref (m); + + /* Now try with the wrong object path */ + m = dbus_message_new_method_call (DBUS_SERVICE_DBUS, + "/com/example/Wrong", DBUS_INTERFACE_DBUS, "UpdateActivationEnvironment"); + + if (m == NULL) + g_error ("OOM"); + + dbus_message_iter_init_append (m, &args_iter); + + /* Append an empty a{ss} (string => string dictionary). */ + if (!dbus_message_iter_open_container (&args_iter, DBUS_TYPE_ARRAY, + "{ss}", &arr_iter) || + !dbus_message_iter_close_container (&args_iter, &arr_iter)) + g_error ("OOM"); + + if (!dbus_connection_send_with_reply (f->left_conn, m, &pc, + DBUS_TIMEOUT_USE_DEFAULT) || + pc == NULL) + g_error ("OOM"); + + dbus_message_unref (m); + m = NULL; + + if (dbus_pending_call_get_completed (pc)) + pending_call_store_reply (pc, &m); + else if (!dbus_pending_call_set_notify (pc, pending_call_store_reply, + &m, NULL)) + g_error ("OOM"); + + while (m == NULL) + test_main_context_iterate (f->ctx, TRUE); + + /* it fails, yielding an error message with one string argument */ + g_assert_cmpint (dbus_message_get_type (m), ==, DBUS_MESSAGE_TYPE_ERROR); + g_assert_cmpstr (dbus_message_get_error_name (m), ==, + DBUS_ERROR_ACCESS_DENIED); + g_assert_cmpstr (dbus_message_get_signature (m), ==, "s"); + + dbus_message_unref (m); +} + static void teardown (Fixture *f, gconstpointer context G_GNUC_UNUSED) @@ -514,6 +599,8 @@ main (int argc, g_test_add ("/echo/limited", Fixture, &limited_config, setup, test_echo, teardown); g_test_add ("/creds", Fixture, NULL, setup, test_creds, teardown); + g_test_add ("/canonical-path/uae", Fixture, NULL, + setup, test_canonical_path_uae, teardown); return g_test_run (); } From eec885de3b4b9559a2f28be7c17bf21ca8d2382f Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 19 Dec 2014 18:51:04 +0000 Subject: [PATCH 4/5] Hardening: only accept Stats function calls at the canonical object path These function calls are not a privilege escalation risk like UpdateActivationEnvironment, but they might provide sensitive information or be enhanced to provide sensitive information in future, so the default system.conf locks them down to root-only. Apply the same canonical-object-path hardening as for UpdateActivationEnvironment. We do not apply the uid check here because they are less dangerous than UpdateActivationEnvironment, and because the ability to unlock these function calls for specific uids is a documented configuration for developers. Reviewed-by: Thiago Macieira [added missing #include; extended commit message -smcv] --- bus/stats.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/bus/stats.c b/bus/stats.c index 24308eb5..20321e5e 100644 --- a/bus/stats.c +++ b/bus/stats.c @@ -29,6 +29,7 @@ #include #include "connection.h" +#include "driver.h" #include "services.h" #include "utils.h" @@ -49,6 +50,9 @@ bus_stats_handle_get_stats (DBusConnection *connection, _DBUS_ASSERT_ERROR_IS_CLEAR (error); + if (!bus_driver_check_message_is_for_us (message, error)) + return FALSE; + context = bus_transaction_get_context (transaction); connections = bus_context_get_connections (context); @@ -131,6 +135,9 @@ bus_stats_handle_get_connection_stats (DBusConnection *caller_connection, _DBUS_ASSERT_ERROR_IS_CLEAR (error); + if (!bus_driver_check_message_is_for_us (message, error)) + return FALSE; + registry = bus_connection_get_registry (caller_connection); if (! dbus_message_get_args (message, error, From abbbf449f17e0a74a5d9a50fb5b074e96e9b7030 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 1 Jan 2015 23:42:41 +0000 Subject: [PATCH 5/5] Prepare release for Monday --- NEWS | 30 ++++++++++++++++++++++++++++-- configure.ac | 4 ++-- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/NEWS b/NEWS index 250aedb1..4fc8c0ff 100644 --- a/NEWS +++ b/NEWS @@ -1,7 +1,33 @@ -D-Bus 1.8.14 (UNRELEASED) +D-Bus 1.8.14 (2015-01-05) == -... +The “40lb of roofing nails” release. + +Security hardening: + +• Do not allow calls to UpdateActivationEnvironment from uids other than + the uid of the dbus-daemon. If a system service installs unsafe + security policy rules that allow arbitrary method calls + (such as CVE-2014-8148) then this prevents memory consumption and + possible privilege escalation via UpdateActivationEnvironment. + + We believe that in practice, privilege escalation here is avoided + by dbus-daemon-launch-helper sanitizing its environment; but + it seems better to be safe. + +• Do not allow calls to UpdateActivationEnvironment or the Stats interface + on object paths other than /org/freedesktop/DBus. Some system services + install unsafe security policy rules that allow arbitrary method calls + to any destination, method and interface with a specified object path; + while less bad than allowing arbitrary method calls, these security + policies are still harmful, since dbus-daemon normally offers the + same API on all object paths and other system services might behave + similarly. + +Other fixes: + +• Add missing initialization so GetExtendedTcpTable doesn't crash on + Windows Vista SP0 (fd.o #77008, Илья А. Ткаченко) D-Bus 1.8.12 (2014-11-24) == diff --git a/configure.ac b/configure.ac index 05b58f76..4bd17b7b 100644 --- a/configure.ac +++ b/configure.ac @@ -3,7 +3,7 @@ AC_PREREQ([2.63]) m4_define([dbus_major_version], [1]) m4_define([dbus_minor_version], [8]) -m4_define([dbus_micro_version], [13]) +m4_define([dbus_micro_version], [14]) m4_define([dbus_version], [dbus_major_version.dbus_minor_version.dbus_micro_version]) AC_INIT([dbus],[dbus_version],[https://bugs.freedesktop.org/enter_bug.cgi?product=dbus],[dbus]) @@ -37,7 +37,7 @@ LT_CURRENT=11 ## increment any time the source changes; set to ## 0 if you increment CURRENT -LT_REVISION=9 +LT_REVISION=10 ## increment if any interfaces have been added; set to 0 ## if any interfaces have been changed or removed. removal has