Attachment 113059 Details for Bug 88810 – [07/11] bus driver: factor out bus_driver_check_caller_is_privileged, and allow root

[patch] [07/11] bus driver: factor out bus_driver_check_caller_is_privileged, and allow root

0007-bus-driver-factor-out-bus_driver_check_caller_is_pri.patch (text/plain), 7.35 KB, created by Simon McVittie on 2015-02-02 19:22:41 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2015-02-02 19:22:41 UTC

Size: 7.35 KB

patch

obsolete

>From 3a71dec87ba582f756b0c93876c71257fd48bdae Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Mon, 26 Jan 2015 19:12:01 +0000
>Subject: [PATCH 07/12] 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
>---
> 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 <deny> 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.1.4
>

Actions: View | Splinter Review

Attachments on bug 88810: 112842 | 112843 | 112844 | 112845 | 112846 | 112847 | 112855 | 113053 | 113054 | 113055 | 113056 | 113057 | 113058 | 113059 | 113060 | 113061 | 113062 | 113063

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.