From 1f716452e702159dc98af00fa7a0c6775ec8de40 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 26 Jan 2015 19:12:01 +0000 Subject: [PATCH] bus driver: factor out bus_driver_check_caller_is_privileged, and allow root Unlike the initial mitigation for CVE-2014-8148, we now allow uid 0 to call UpdateActivationEnvironment. There's no point in root doing that, but there's also no reason why it's particularly bad - if an attacker is uid 0 we've already lost - and it simplifies use of this function for future things that do want to be callable by root, like BecomeMonitor for #46787. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=88810 Reviewed-by: Philip Withnall --- bus/driver.c | 136 ++++++++++++++++++++++++++++++++++++++----------- test/uid-permissions.c | 6 +-- 2 files changed, 110 insertions(+), 32 deletions(-) diff --git a/bus/driver.c b/bus/driver.c index 952061c..21d4a0a 100644 --- a/bus/driver.c +++ b/bus/driver.c @@ -82,6 +82,107 @@ bus_driver_get_conn_helper (DBusConnection *connection, return conn; } +/* + * Log a security warning and set error unless the uid of the connection + * is either the uid of this process, or on Unix, uid 0 (root). + * + * This is intended to be a second line of defence after rules, + * to mitigate incorrect system bus security policy configuration files + * like the ones in CVE-2014-8148 and CVE-2014-8156, and (if present) + * LSM rules; so it doesn't need to be perfect, but as long as we have + * potentially dangerous functionality in the system bus, it does need + * to exist. + */ +static dbus_bool_t +bus_driver_check_caller_is_privileged (DBusConnection *connection, + BusTransaction *transaction, + DBusMessage *message, + DBusError *error) +{ +#ifdef DBUS_UNIX + unsigned long uid; + + if (!dbus_connection_get_unix_user (connection, &uid)) + { + const char *method = dbus_message_get_member (message); + + /* Yes this repetition is pretty horrible, but there's no + * bus_context_log_valist() or dbus_set_error_valist() or + * bus_context_log_literal() or dbus_set_error_literal(). + */ + bus_context_log (bus_transaction_get_context (transaction), + DBUS_SYSTEM_LOG_SECURITY, + "rejected attempt to call %s by unknown uid", method); + dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED, + "rejected attempt to call %s by unknown uid", method); + return FALSE; + } + + /* I'm writing it in this slightly strange form so that it's more + * obvious that this security-sensitive code is correct. + */ + if (_dbus_unix_user_is_process_owner (uid)) + { + /* OK */ + } + else if (uid == 0) + { + /* OK */ + } + else + { + const char *method = dbus_message_get_member (message); + + bus_context_log (bus_transaction_get_context (transaction), + DBUS_SYSTEM_LOG_SECURITY, + "rejected attempt to call %s by uid %lu", method, uid); + dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED, + "rejected attempt to call %s by uid %lu", method, uid); + return FALSE; + } + + return TRUE; +#elif DBUS_WIN + char *windows_sid = NULL; + dbus_bool_t ret = FALSE; + + if (!dbus_connection_get_windows_user (connection, &windows_sid)) + { + const char *method = dbus_message_get_member (message); + + bus_context_log (bus_transaction_get_context (transaction), + DBUS_SYSTEM_LOG_SECURITY, + "rejected attempt to call %s by unknown uid", method); + dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED, + "rejected attempt to call %s by unknown uid", method); + goto out; + } + + if (!_dbus_windows_user_is_process_owner (windows_sid)) + { + const char *method = dbus_message_get_member (message); + + bus_context_log (bus_transaction_get_context (transaction), + DBUS_SYSTEM_LOG_SECURITY, + "rejected attempt to call %s by uid %s", method, windows_sid); + dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED, + "rejected attempt to call %s by uid %s", method, windows_sid); + goto out; + } + + ret = TRUE; +out: + dbus_free (windows_sid); + return ret; +#else + /* make sure we fail closed in the hypothetical case that we are neither + * Unix nor Windows */ + dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED, + "please teach bus/driver.c how uids work on this platform"); + return FALSE; +#endif +} + static dbus_bool_t bus_driver_send_welcome_message (DBusConnection *connection, DBusMessage *hello_message, BusTransaction *transaction, @@ -884,35 +985,12 @@ bus_driver_handle_update_activation_environment (DBusConnection *connection, #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; - } + * escalation so let's be extra-careful: do not allow the sysadmin + * to shoot themselves in the foot. + */ + if (!bus_driver_check_caller_is_privileged (connection, transaction, + message, error)) + return FALSE; } #endif diff --git a/test/uid-permissions.c b/test/uid-permissions.c index 1bb1a31..407b530 100644 --- a/test/uid-permissions.c +++ b/test/uid-permissions.c @@ -164,10 +164,10 @@ teardown (Fixture *f, test_main_context_unref (f->ctx); } -static Config root_fail_config = { +static Config root_ok_config = { "valid-config-files/multi-user.conf", TEST_USER_ROOT, - FALSE + TRUE }; static Config messagebus_ok_config = { @@ -189,7 +189,7 @@ main (int argc, g_test_init (&argc, &argv, NULL); g_test_bug_base ("https://bugs.freedesktop.org/show_bug.cgi?id="); - g_test_add ("/uid-permissions/uae/root", Fixture, &root_fail_config, + g_test_add ("/uid-permissions/uae/root", Fixture, &root_ok_config, setup, test_uae, teardown); g_test_add ("/uid-permissions/uae/messagebus", Fixture, &messagebus_ok_config, setup, test_uae, teardown); -- 2.7.4