Attachment 128111 Details for Bug 98666 – Do not auto-activate services if we could not send a message

[patch] Do not auto-activate services if we could not send a message

0006-Do-not-auto-activate-services-if-we-could-not-send-a.patch (text/plain), 15.51 KB, created by Simon McVittie on 2016-11-21 21:21:00 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2016-11-21 21:21:00 UTC

Size: 15.51 KB

patch

obsolete

>From 9d017bf30fd4d0acbb501f8feded4478e2bc6d70 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Mon, 21 Nov 2016 20:56:55 +0000
>Subject: [PATCH 06/10] Do not auto-activate services if we could not send a
> message
>
>We specifically do not check recipient policies, because
>the recipient policy is based on properties of the
>recipient process (in particular, its uid), which we do
>not necessarily know until we have already started it.
>
>In this initial implementation we do not check LSMs either,
>because we cannot know what LSM context the recipient process
>is going to have. However, LSM support will need to be added
>to make this feature useful, because StartServiceByName is
>normally allowed in non-LSM environments, and is more
>powerful than auto-activation anyway.
>
>The StartServiceByName method does not go through this check,
>because if access to that method has been granted, then
>it's somewhat obvious that you can start arbitrary services.
>---
> bus/activation.c     | 22 ++++++++++++++++++++--
> bus/apparmor.c       |  5 +++++
> bus/apparmor.h       |  1 +
> bus/bus.c            | 17 +++++++++++++++--
> bus/bus.h            |  2 ++
> bus/connection.c     |  3 ++-
> bus/dispatch.c       | 15 +++++++++------
> bus/policy.c         |  5 +++++
> bus/selinux.c        |  8 +++++++-
> bus/selinux.h        |  1 +
> test/sd-activation.c | 38 +++-----------------------------------
> 11 files changed, 70 insertions(+), 47 deletions(-)
>
>diff --git a/bus/activation.c b/bus/activation.c
>index 1e59190..e131a80 100644
>--- a/bus/activation.c
>+++ b/bus/activation.c
>@@ -64,7 +64,7 @@ typedef struct
>   DBusHashTable *entries;
> } BusServiceDirectory;
> 
>-typedef struct
>+struct BusActivationEntry
> {
>   int refcount;
>   char *name;
>@@ -74,7 +74,7 @@ typedef struct
>   unsigned long mtime;
>   BusServiceDirectory *s_dir;
>   char *filename;
>-} BusActivationEntry;
>+};
> 
> typedef struct BusPendingActivationEntry BusPendingActivationEntry;
> 
>@@ -1691,6 +1691,24 @@ bus_activation_activate_service (BusActivation  *activation,
>         return FALSE;
>     }
> 
>+  if (auto_activation &&
>+      entry != NULL &&
>+      !bus_context_check_security_policy (activation->context,
>+        transaction,
>+        connection, /* sender */
>+        NULL, /* addressed recipient */
>+        NULL, /* proposed recipient */
>+        activation_message,
>+        entry,
>+        error))
>+    {
>+      _DBUS_ASSERT_ERROR_IS_SET (error);
>+      _dbus_verbose ("activation not authorized: %s: %s\n",
>+          error != NULL ? error->name : "(error ignored)",
>+          error != NULL ? error->message : "(error ignored)");
>+      return FALSE;
>+    }
>+
>   /* Bypass the registry lookup if we're auto-activating, bus_dispatch would not
>    * call us if the service is already active.
>    */
>diff --git a/bus/apparmor.c b/bus/apparmor.c
>index c679ac5..1701ca4 100644
>--- a/bus/apparmor.c
>+++ b/bus/apparmor.c
>@@ -739,6 +739,7 @@ bus_apparmor_allows_send (DBusConnection     *sender,
>                           const char         *error_name,
>                           const char         *destination,
>                           const char         *source,
>+                          BusActivationEntry *activation_entry,
>                           DBusError          *error)
> {
> #ifdef HAVE_APPARMOR
>@@ -755,6 +756,10 @@ bus_apparmor_allows_send (DBusConnection     *sender,
>   if (!apparmor_enabled)
>     return TRUE;
> 
>+  /* We do not mediate activation attempts yet. */
>+  if (activation_entry != NULL)
>+    return TRUE;
>+
>   _dbus_assert (sender != NULL);
> 
>   src_con = bus_connection_dup_apparmor_confinement (sender);
>diff --git a/bus/apparmor.h b/bus/apparmor.h
>index 18f3ee7..ed465f7 100644
>--- a/bus/apparmor.h
>+++ b/bus/apparmor.h
>@@ -56,6 +56,7 @@ dbus_bool_t bus_apparmor_allows_send (DBusConnection     *sender,
>                                       const char         *error_name,
>                                       const char         *destination,
>                                       const char         *source,
>+                                      BusActivationEntry *activation_entry,
>                                       DBusError          *error);
> 
> dbus_bool_t bus_apparmor_allows_eavesdropping (DBusConnection     *connection,
>diff --git a/bus/bus.c b/bus/bus.c
>index df8239f..b27b4a5 100644
>--- a/bus/bus.c
>+++ b/bus/bus.c
>@@ -1511,7 +1511,11 @@ complain_about_message (BusContext     *context,
>  *
>  * sender is the sender of the message.
>  *
>- * NULL for proposed_recipient or sender definitely means the bus driver.
>+ * NULL for sender definitely means the bus driver.
>+ *
>+ * NULL for proposed_recipient may mean the bus driver, or may mean
>+ * we are checking whether service-activation is allowed as a first
>+ * pass before all details of the activated service are known.
>  *
>  * NULL for addressed_recipient may mean the bus driver, or may mean
>  * no destination was specified in the message (e.g. a signal).
>@@ -1523,6 +1527,7 @@ bus_context_check_security_policy (BusContext     *context,
>                                    DBusConnection *addressed_recipient,
>                                    DBusConnection *proposed_recipient,
>                                    DBusMessage    *message,
>+                                   BusActivationEntry *activation_entry,
>                                    DBusError      *error)
> {
>   const char *src, *dest;
>@@ -1543,6 +1548,7 @@ bus_context_check_security_policy (BusContext     *context,
>                 (sender == NULL && !bus_connection_is_active (proposed_recipient)));
>   _dbus_assert (type == DBUS_MESSAGE_TYPE_SIGNAL ||
>                 addressed_recipient != NULL ||
>+                activation_entry != NULL ||
>                 strcmp (dest, DBUS_SERVICE_DBUS) == 0);
> 
>   switch (type)
>@@ -1608,7 +1614,9 @@ bus_context_check_security_policy (BusContext     *context,
> 				    dbus_message_get_interface (message),
> 				    dbus_message_get_member (message),
> 				    dbus_message_get_error_name (message),
>-				    dest ? dest : DBUS_SERVICE_DBUS, error))
>+				    dest ? dest : DBUS_SERVICE_DBUS,
>+				    activation_entry,
>+				    error))
>         {
>           if (error != NULL && !dbus_error_is_set (error))
>             {
>@@ -1637,6 +1645,7 @@ bus_context_check_security_policy (BusContext     *context,
>                                      dbus_message_get_error_name (message),
>                                      dest ? dest : DBUS_SERVICE_DBUS,
>                                      src ? src : DBUS_SERVICE_DBUS,
>+                                     activation_entry,
>                                      error))
>         return FALSE;
> 
>@@ -1769,6 +1778,10 @@ bus_context_check_security_policy (BusContext     *context,
>   /* Record that we will allow a reply here in the future (don't
>    * bother if the recipient is the bus or this is an eavesdropping
>    * connection). Only the addressed recipient may reply.
>+   *
>+   * This isn't done for activation attempts because they have no addressed
>+   * or proposed recipient; when we check whether to actually deliver the
>+   * message, later, we'll record the reply expectation at that point.
>    */
>   if (type == DBUS_MESSAGE_TYPE_METHOD_CALL &&
>       sender &&
>diff --git a/bus/bus.h b/bus/bus.h
>index ca42533..2e0de82 100644
>--- a/bus/bus.h
>+++ b/bus/bus.h
>@@ -44,6 +44,7 @@ typedef struct BusOwner		BusOwner;
> typedef struct BusTransaction   BusTransaction;
> typedef struct BusMatchmaker    BusMatchmaker;
> typedef struct BusMatchRule     BusMatchRule;
>+typedef struct BusActivationEntry BusActivationEntry;
> 
> typedef struct
> {
>@@ -141,6 +142,7 @@ dbus_bool_t       bus_context_check_security_policy              (BusContext
>                                                                   DBusConnection   *addressed_recipient,
>                                                                   DBusConnection   *proposed_recipient,
>                                                                   DBusMessage      *message,
>+                                                                  BusActivationEntry *activation_entry,
>                                                                   DBusError        *error);
> void              bus_context_check_all_watches                  (BusContext       *context);
> 
>diff --git a/bus/connection.c b/bus/connection.c
>index 58fc219..4b53cd0 100644
>--- a/bus/connection.c
>+++ b/bus/connection.c
>@@ -2387,7 +2387,8 @@ bus_transaction_send_from_driver (BusTransaction *transaction,
>    */
>   if (!bus_context_check_security_policy (bus_transaction_get_context (transaction),
>                                           transaction,
>-                                          NULL, connection, connection, message, &error))
>+                                          NULL, connection, connection,
>+                                          message, NULL, &error))
>     {
>       if (!bus_transaction_capture_error_reply (transaction, connection,
>                                                 &error, message))
>diff --git a/bus/dispatch.c b/bus/dispatch.c
>index 021dfc4..620fd36 100644
>--- a/bus/dispatch.c
>+++ b/bus/dispatch.c
>@@ -70,6 +70,7 @@ send_one_message (DBusConnection *connection,
>                                           addressed_recipient,
>                                           connection,
>                                           message,
>+                                          NULL,
>                                           &stack_error))
>     {
>       if (!bus_transaction_capture_error_reply (transaction, sender,
>@@ -147,7 +148,7 @@ bus_dispatch_matches (BusTransaction *transaction,
>       if (!bus_context_check_security_policy (context, transaction,
>                                               sender, addressed_recipient,
>                                               addressed_recipient,
>-                                              message, error))
>+                                              message, NULL, error))
>         return FALSE;
> 
>       if (dbus_message_contains_unix_fds (message) &&
>@@ -380,7 +381,8 @@ bus_dispatch (DBusConnection *connection,
>         }
> 
>       if (!bus_context_check_security_policy (context, transaction,
>-                                              connection, NULL, NULL, message, &error))
>+                                              connection, NULL, NULL, message,
>+                                              NULL, &error))
>         {
>           _dbus_verbose ("Security policy rejected message\n");
>           goto out;
>@@ -426,12 +428,13 @@ bus_dispatch (DBusConnection *connection,
>               goto out;
>             }
> 
>-          /* We can't do the security policy check here, since the addressed
>-           * recipient service doesn't exist yet. We do it before sending the
>-           * message after the service has been created.
>-           */
>           activation = bus_connection_get_activation (connection);
> 
>+          /* This will do as much of a security policy check as it can.
>+           * We can't do the full security policy check here, since the
>+           * addressed recipient service doesn't exist yet. We do it before
>+           * sending the message after the service has been created.
>+           */
>           if (!bus_activation_activate_service (activation, connection, transaction, TRUE,
>                                                 message, service_name, &error))
>             {
>diff --git a/bus/policy.c b/bus/policy.c
>index 082f385..dd0ac86 100644
>--- a/bus/policy.c
>+++ b/bus/policy.c
>@@ -993,6 +993,11 @@ bus_client_policy_check_can_send (BusClientPolicy *policy,
>            * message bus itself, we check the strings in that case as
>            * built-in services don't have a DBusConnection but messages
>            * to them have a destination service name.
>+           *
>+           * Similarly, receiver can be NULL when we're deciding whether
>+           * activation should be allowed; we make the authorization decision
>+           * on the assumption that the activated service will have the
>+           * requested name and no others.
>            */
>           if (receiver == NULL)
>             {
>diff --git a/bus/selinux.c b/bus/selinux.c
>index 16791c8..e484be6 100644
>--- a/bus/selinux.c
>+++ b/bus/selinux.c
>@@ -553,6 +553,7 @@ bus_selinux_allows_send (DBusConnection     *sender,
> 			 const char         *member,
> 			 const char         *error_name,
> 			 const char         *destination,
>+			 BusActivationEntry *activation_entry,
> 			 DBusError          *error)
> {
> #ifdef HAVE_SELINUX
>@@ -566,6 +567,10 @@ bus_selinux_allows_send (DBusConnection     *sender,
>   if (!selinux_enabled)
>     return TRUE;
> 
>+  /* We do not mediate activation attempts yet. */
>+  if (activation_entry)
>+    return TRUE;
>+
>   if (!sender || !dbus_connection_get_unix_process_id (sender, &spid))
>     spid = 0;
>   if (!proposed_recipient || !dbus_connection_get_unix_process_id (proposed_recipient, &tpid))
>@@ -633,7 +638,8 @@ bus_selinux_allows_send (DBusConnection     *sender,
>     }
> 
>   sender_sid = bus_connection_get_selinux_id (sender);
>-  /* A NULL proposed_recipient means the bus itself. */
>+
>+  /* A NULL proposed_recipient with no activation entry means the bus itself. */
>   if (proposed_recipient)
>     recipient_sid = bus_connection_get_selinux_id (proposed_recipient);
>   else
>diff --git a/bus/selinux.h b/bus/selinux.h
>index e44c97e..5252b18 100644
>--- a/bus/selinux.h
>+++ b/bus/selinux.h
>@@ -61,6 +61,7 @@ dbus_bool_t bus_selinux_allows_send            (DBusConnection *sender,
> 						const char     *member,
> 						const char     *error_name,
> 						const char     *destination,
>+						BusActivationEntry *activation_entry,
> 						DBusError      *error);
> 
> BusSELinuxID* bus_selinux_init_connection_id (DBusConnection *connection,
>diff --git a/test/sd-activation.c b/test/sd-activation.c
>index 6d52987..9b2a5bb 100644
>--- a/test/sd-activation.c
>+++ b/test/sd-activation.c
>@@ -575,44 +575,12 @@ test_deny_send (Fixture *f,
>   dbus_connection_send (f->caller, m, NULL);
>   dbus_message_unref (m);
> 
>-  /* The fake systemd connects to the bus. */
>-  f->systemd = test_connect_to_bus (f->ctx, f->address);
>-  if (!dbus_connection_add_filter (f->systemd, systemd_filter, f, NULL))
>-    g_error ("OOM");
>-  f->systemd_filter_added = TRUE;
>-  f->systemd_name = dbus_bus_get_unique_name (f->systemd);
>-  take_well_known_name (f, f->systemd, "org.freedesktop.systemd1");
>-
>-  /* It gets its activation request. */
>-  while (f->caller_message == NULL && f->systemd_message == NULL)
>-    test_main_context_iterate (f->ctx, TRUE);
>-
>-  g_assert (f->caller_message == NULL);
>-  g_assert (f->systemd_message != NULL);
>-
>-  m = f->systemd_message;
>-  f->systemd_message = NULL;
>-  assert_signal (m, DBUS_SERVICE_DBUS, DBUS_PATH_DBUS,
>-      "org.freedesktop.systemd1.Activator", "ActivationRequest", "s",
>-      "org.freedesktop.systemd1");
>-  dbus_message_unref (m);
>-
>-  /* systemd starts the activatable service. */
>-  f->activated = test_connect_to_bus (f->ctx, f->address);
>-  if (!dbus_connection_add_filter (f->activated, activated_filter,
>-        f, NULL))
>-    g_error ("OOM");
>-  f->activated_filter_added = TRUE;
>-  f->activated_name = dbus_bus_get_unique_name (f->activated);
>-  take_well_known_name (f, f->activated, "com.example.SendDenied");
>+  /* Even before the fake systemd connects to the bus, we get an error
>+   * back: activation is not allowed. */
> 
>-  /* We re-do the message matching, and now the message is
>-   * forbidden by the receive policy. */
>-  while (f->activated_message == NULL && f->caller_message == NULL)
>+  while (f->caller_message == NULL)
>     test_main_context_iterate (f->ctx, TRUE);
> 
>-  g_assert (f->activated_message == NULL);
>-
>   m = f->caller_message;
>   f->caller_message = NULL;
>   assert_error_reply (m, DBUS_SERVICE_DBUS, f->caller_name,
>-- 
>2.10.2
>

Actions: View | Splinter Review

Attachments on bug 98666: 127875 | 127876 | 127877 | 127878 | 127879 | 127880 | 127881 | 128111 | 128112 | 128113 | 128114 | 128115 | 128142 | 128243 | 128259

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.