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.

Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: Philip Withnall <philip.withnall@collabora.co.uk>
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=98666

Change-Id: I53ff4f6d02e631fcd09bf1c5c306b8828f075963

diff --git a/bus/activation.c b/bus/activation.c
index 6d8a7c78baec6668a675a8167e2e591e22bcb6c9..99b3554ae7c450207d05ab2c7d2c5459a1492ca9 100644 (file)
--- a/bus/activation.c
+++ b/bus/activation.c
@@ -65,7 +65,7 @@ typedef struct
   DBusHashTable *entries;
 } BusServiceDirectory;
 
-typedef struct
+struct BusActivationEntry
 {
   int refcount;
   char *name;
@@ -75,7 +75,7 @@ typedef struct
   unsigned long mtime;
   BusServiceDirectory *s_dir;
   char *filename;
-} BusActivationEntry;
+};
 
 typedef struct BusPendingActivationEntry BusPendingActivationEntry;
 
@@ -1804,6 +1804,31 @@ bus_activation_activate_service (BusActivation  *activation,
   if (!entry)
     return BUS_RESULT_FALSE;
 
+  if (auto_activation && entry != NULL)
+    {
+      switch (bus_context_check_security_policy (activation->context,
+        transaction,
+        connection, /* sender */
+        NULL, /* addressed recipient */
+        NULL, /* proposed recipient */
+        activation_message,
+        entry,
+        error,
+        deferred_message))
+        {
+        case BUS_RESULT_TRUE:
+            break;
+        case BUS_RESULT_FALSE:
+          _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 BUS_RESULT_FALSE;
+        case BUS_RESULT_LATER:
+          return BUS_RESULT_LATER;
+        }
+      }
+
   /* 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 3ba84d8da1d18dbedba781ccd176f2b13228aca8..46725dbf54d9ff8e7bebaa2a312c05d039d061d6 100644 (file)
--- a/bus/apparmor.c
+++ b/bus/apparmor.c
@@ -737,6 +737,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
@@ -753,6 +754,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 18f3ee79550e5adfdd4709d553d098cd7edcbde4..ed465f7113c0e4183a2d5aef448878fc16f1918a 100644 (file)
--- 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 144c0df96a75220826579d71156468f1027f7f18..4943b9f4cda7c6e9fe019539e8395d2f46d7763c 100644 (file)
--- a/bus/bus.c
+++ b/bus/bus.c
@@ -1524,7 +1524,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).
@@ -1536,6 +1540,7 @@ bus_context_check_security_policy (BusContext          *context,
                                    DBusConnection      *addressed_recipient,
                                    DBusConnection      *proposed_recipient,
                                    DBusMessage         *message,
+                                   BusActivationEntry  *activation_entry,
                                    DBusError           *error,
                                    BusDeferredMessage **deferred_message)
 {
@@ -1558,6 +1563,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)
@@ -1623,7 +1629,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))
             {
@@ -1652,6 +1660,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;
 
@@ -1795,6 +1804,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 95ca9a8e55e0191fd91312c7ecf87430313a1e0e..6382b18843e43efa8cfe38e2b138d8e242a753da 100644 (file)
--- 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 BusCheck           BusCheck;
 typedef struct BusDeferredMessage BusDeferredMessage;
 typedef struct BusCynara          BusCynara;
@@ -158,6 +159,7 @@ BusResult         bus_context_check_security_policy              (BusContext
                                                                   DBusConnection      *addressed_recipient,
                                                                   DBusConnection      *proposed_recipient,
                                                                   DBusMessage         *message,
+                                                                  BusActivationEntry  *activation_entry,
                                                                   DBusError           *error,
                                                                   BusDeferredMessage **deferred_message);
 

diff --git a/bus/check.c b/bus/check.c
index 9bbca639d116ce7d07d8c8874223b0221330e021..2aeb69e5f32f225a695e29d87080f65ecae5d45d 100644 (file)
--- a/bus/check.c
+++ b/bus/check.c
@@ -470,7 +470,7 @@ bus_deferred_message_dispatch (BusDeferredMessage *deferred_message)
                                                         deferred_message->sender,
                                                         deferred_message->addressed_recipient,
                                                         deferred_message->proposed_recipient,
-                                                        deferred_message->message, NULL,
+                                                        deferred_message->message, NULL, NULL,
                                                         &deferred_message2);
 
             if (result == BUS_RESULT_LATER)

diff --git a/bus/connection.c b/bus/connection.c
index 2173ce98ae9f06ee9217212a545dc8dc9b1bd3f9..b4a2995cc26b873f9bdfac0bd63b50c81e51aabb 100644 (file)
--- a/bus/connection.c
+++ b/bus/connection.c
@@ -2340,7 +2340,8 @@ bus_transaction_send_from_driver (BusTransaction *transaction,
    */
   switch (bus_context_check_security_policy (bus_transaction_get_context (transaction),
                                              transaction,
-                                             NULL, connection, connection, message, &error,
+                                             NULL, connection, connection,
+                                             message, NULL, &error,
                                              &deferred_message))
     {
     case BUS_RESULT_TRUE:

diff --git a/bus/dispatch.c b/bus/dispatch.c
index 67763b9a6d1873d3cae79742c610606aad74d3be..f4dea605f5ec33b44e0669d6eeefb50e275658a0 100644 (file)
--- a/bus/dispatch.c
+++ b/bus/dispatch.c
@@ -71,7 +71,7 @@ send_one_message (DBusConnection *connection,
   BusResult result;
 
   result = bus_context_check_security_policy (context, transaction, sender, addressed_recipient,
-      connection, message, &stack_error, &deferred_message);
+      connection, message, NULL, &stack_error, &deferred_message);
 
   if (result == BUS_RESULT_FALSE)
     {
@@ -211,7 +211,7 @@ bus_dispatch_matches (BusTransaction     *transaction,
 
       if (result == BUS_RESULT_LATER)
         result = bus_context_check_security_policy(context, transaction,
-            sender, addressed_recipient, addressed_recipient, message, error,
+            sender, addressed_recipient, addressed_recipient, message, NULL, error,
             &deferred_message);
 
       switch (result)
@@ -495,7 +495,8 @@ bus_dispatch (DBusConnection *connection,
       BusDeferredMessage *deferred_message;
 
       switch (bus_context_check_security_policy (context, transaction,
-                                                 connection, NULL, NULL, message, &error,
+                                                 connection, NULL, NULL, message,
+                                                 NULL, &error,
                                                  &deferred_message))
         {
         case BUS_RESULT_TRUE:
@@ -551,12 +552,13 @@ bus_dispatch (DBusConnection *connection,
           BusActivation *activation;
           BusDeferredMessage *deferred_message;
 
-          /* 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.
+           */
           switch (bus_activation_activate_service (activation, connection, transaction, TRUE,
                                                 message, service_name, &error,
                                                 &deferred_message))

diff --git a/bus/policy.c b/bus/policy.c
index 932a9b46f910e9f2977d77899d88b1856c0443c0..3d4f54774dfc03908027811ccca6fc7e382e41ad 100644 (file)
--- a/bus/policy.c
+++ b/bus/policy.c
@@ -1007,6 +1007,11 @@ bus_client_policy_check_can_send (DBusConnection      *sender,
            * 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 ef627c8c0cb21bab698496d06520904b11ba0944..be66d4f0602f2217d05ea7eb836a882297fea9b2 100644 (file)
--- a/bus/selinux.c
+++ b/bus/selinux.c
@@ -551,6 +551,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
@@ -564,6 +565,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))
@@ -631,7 +636,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 e44c97ede3d77169fc739b34a11effeb550ea380..5252b18988b6c6ce3263e71a1d86291e2b832c5d 100644 (file)
--- 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 6d5298701e0ec0676cd2c511a912f412add92622..9b2a5bb540c79f2275d25307f00f04343c5f081b 100644 (file)
--- 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,