Mediate auto-activation attempts through AppArmor

Because the recipient process is not yet available, we have to make some
assumption about its AppArmor profile. Parsing the first word of
the Exec value and then chasing symlinks seems like too much magic,
so I've gone for something more explicit. If the .service file contains

AssumedAppArmorLabel=/foo/bar

then we will do the AppArmor query on the assumption that the recipient
AppArmor label will be as stated. Otherwise, we will do a query
with an unspecified label, which means that AppArmor rules that do
specify a peer label will never match it.

Regardless of the result of this query, we will do an independent
AppArmor query when the activation has actually happened, this time
with the correct peer label; that second query will still be used
to decide whether to deliver the message. As a result, if this change
has any effect, it is to make the bus more restrictive; it does not
allow anything that would previously have been denied.

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

bus/activation.c

@@ -71,6 +71,7 @@ struct BusActivationEntry
   char *exec;
   char *user;
   char *systemd_service;
+  char *assumed_apparmor_label;
   unsigned long mtime;
   BusServiceDirectory *s_dir;
   char *filename;
@@ -244,6 +245,7 @@ bus_activation_entry_unref (BusActivationEntry *entry)
   dbus_free (entry->user);
   dbus_free (entry->filename);
   dbus_free (entry->systemd_service);
+  dbus_free (entry->assumed_apparmor_label);
 
   dbus_free (entry);
 }
@@ -256,6 +258,7 @@ update_desktop_file_entry (BusActivation       *activation,
                            DBusError           *error)
 {
   char *name, *exec, *user, *exec_tmp, *systemd_service;
+  char *assumed_apparmor_label;
   BusActivationEntry *entry;
   DBusStat stat_buf;
   DBusString file_path;
@@ -272,6 +275,7 @@ update_desktop_file_entry (BusActivation       *activation,
   exec_tmp = NULL;
   entry = NULL;
   systemd_service = NULL;
+  assumed_apparmor_label = NULL;
 
   dbus_error_init (&tmp_error);
 
@@ -367,6 +371,28 @@ update_desktop_file_entry (BusActivation       *activation,
         }
     }
 
+  /* assumed AppArmor label is never required */
+  if (!bus_desktop_file_get_string (desktop_file,
+                                    DBUS_SERVICE_SECTION,
+                                    DBUS_SERVICE_ASSUMED_APPARMOR_LABEL,
+                                    &assumed_apparmor_label, &tmp_error))
+    {
+      _DBUS_ASSERT_ERROR_IS_SET (&tmp_error);
+      /* if we got OOM, then exit */
+      if (dbus_error_has_name (&tmp_error, DBUS_ERROR_NO_MEMORY))
+        {
+          dbus_move_error (&tmp_error, error);
+          goto out;
+        }
+      else
+        {
+          /* if we have error because we didn't find anything then continue */
+          dbus_error_free (&tmp_error);
+          dbus_free (assumed_apparmor_label);
+          assumed_apparmor_label = NULL;
+        }
+    }
+
   _DBUS_ASSERT_ERROR_IS_CLEAR (&tmp_error);
 
   entry = _dbus_hash_table_lookup_string (s_dir->entries,
@@ -395,6 +421,7 @@ update_desktop_file_entry (BusActivation       *activation,
       entry->exec = exec;
       entry->user = user;
       entry->systemd_service = systemd_service;
+      entry->assumed_apparmor_label = assumed_apparmor_label;
       entry->refcount = 1;
 
       /* ownership has been transferred to entry, do not free separately */
@@ -402,6 +429,7 @@ update_desktop_file_entry (BusActivation       *activation,
       exec = NULL;
       user = NULL;
       systemd_service = NULL;
+      assumed_apparmor_label = NULL;
 
       entry->s_dir = s_dir;
       entry->filename = _dbus_strdup (_dbus_string_get_const_data (filename));
@@ -459,6 +487,10 @@ update_desktop_file_entry (BusActivation       *activation,
       entry->systemd_service = systemd_service;
       systemd_service = NULL;
 
+      dbus_free (entry->assumed_apparmor_label);
+      entry->assumed_apparmor_label = assumed_apparmor_label;
+      assumed_apparmor_label = NULL;
+
       if (!_dbus_hash_table_insert_string (activation->entries,
                                            entry->name, bus_activation_entry_ref(entry)))
         {
@@ -481,6 +513,7 @@ out:
   dbus_free (exec);
   dbus_free (user);
   dbus_free (systemd_service);
+  dbus_free (assumed_apparmor_label);
   _dbus_string_free (&file_path);
 
   if (entry)
@@ -2269,6 +2302,12 @@ dbus_activation_systemd_failure (BusActivation *activation,
   return TRUE;
 }
 
+const char *
+bus_activation_entry_get_assumed_apparmor_label (BusActivationEntry *entry)
+{
+  return entry->assumed_apparmor_label;
+}
+
 #ifdef DBUS_ENABLE_EMBEDDED_TESTS
 
 #include <stdio.h>

bus/activation.h

@@ -64,5 +64,6 @@ dbus_bool_t    bus_activation_send_pending_auto_activation_messages (BusActivati
 								     BusService        *service,
 								     BusTransaction    *transaction);
 
+const char *bus_activation_entry_get_assumed_apparmor_label (BusActivationEntry *entry);
 
 #endif /* BUS_ACTIVATION_H */

bus/apparmor.c

@@ -46,6 +46,7 @@
 #include <libaudit.h>
 #endif /* HAVE_LIBAUDIT */
 
+#include "activation.h"
 #include "audit.h"
 #include "connection.h"
 #include "utils.h"
@@ -752,14 +753,12 @@ bus_apparmor_allows_send (DBusConnection     *sender,
   int len, res, src_errno = 0, dst_errno = 0;
   uint32_t src_perm = AA_DBUS_SEND, dst_perm = AA_DBUS_RECEIVE;
   const char *msgtypestr = dbus_message_type_to_string(msgtype);
+  const char *dst_label = NULL;
+  const char *dst_mode = NULL;
 
   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);
@@ -768,12 +767,22 @@ bus_apparmor_allows_send (DBusConnection     *sender,
     {
       dst_con = bus_connection_dup_apparmor_confinement (proposed_recipient);
     }
+  else if (activation_entry != NULL)
+    {
+      dst_label = bus_activation_entry_get_assumed_apparmor_label (activation_entry);
+    }
   else
     {
       dst_con = bus_con;
       bus_apparmor_confinement_ref (dst_con);
     }
 
+  if (dst_con != NULL)
+    {
+      dst_label = dst_con->label;
+      dst_mode = dst_con->mode;
+    }
+
   /* map reply messages to initial send and receive permission. That is
    * permission to receive a message from X grants permission to reply to X.
    * And permission to send a message to Y grants permission to receive a reply
@@ -801,7 +810,7 @@ bus_apparmor_allows_send (DBusConnection     *sender,
         goto oom;
 
       if (!build_message_query (&qstr, src_con->label, bustype, destination,
-                                dst_con->label, path, interface, member))
+                                dst_label, path, interface, member))
         {
           _dbus_string_free (&qstr);
           goto oom;
@@ -820,7 +829,11 @@ bus_apparmor_allows_send (DBusConnection     *sender,
         }
     }
 
-  if (is_unconfined (dst_con->label, dst_con->mode))
+  /* When deciding whether we can activate a service, we only check that
+   * we are allowed to send a message to it, not that it is allowed to
+   * receive that message from us.
+   */
+  if (activation_entry != NULL || is_unconfined (dst_label, dst_mode))
     {
       dst_allow = TRUE;
       dst_audit = FALSE;
@@ -830,7 +843,7 @@ bus_apparmor_allows_send (DBusConnection     *sender,
       if (!_dbus_string_init (&qstr))
         goto oom;
 
-      if (!build_message_query (&qstr, dst_con->label, bustype, source,
+      if (!build_message_query (&qstr, dst_label, bustype, source,
                                 src_con->label, path, interface, member))
         {
           _dbus_string_free (&qstr);
@@ -853,7 +866,7 @@ bus_apparmor_allows_send (DBusConnection     *sender,
   /* Don't fail operations on profiles in complain mode */
   if (modestr_is_complain (src_con->mode))
     src_allow = TRUE;
-  if (modestr_is_complain (dst_con->mode))
+  if (modestr_is_complain (dst_mode))
     dst_allow = TRUE;
 
   if (!src_allow || !dst_allow)
@@ -924,8 +937,8 @@ bus_apparmor_allows_send (DBusConnection     *sender,
           !_dbus_append_pair_uint (&auxdata, "peer_pid", pid))
         goto oom;
 
-      if (dst_con->label &&
-          !_dbus_append_pair_str (&auxdata, "peer_label", dst_con->label))
+      if (dst_label &&
+          !_dbus_append_pair_str (&auxdata, "peer_label", dst_label))
         goto oom;
 
       if (src_errno && !_dbus_append_pair_str (&auxdata, "info", strerror (src_errno)))
@@ -952,8 +965,8 @@ bus_apparmor_allows_send (DBusConnection     *sender,
           !_dbus_append_pair_uint (&auxdata, "pid", pid))
         goto oom;
 
-      if (dst_con->label &&
-          !_dbus_append_pair_str (&auxdata, "label", dst_con->label))
+      if (dst_label &&
+          !_dbus_append_pair_str (&auxdata, "label", dst_label))
         goto oom;
 
       if (sender && dbus_connection_get_unix_process_id (sender, &pid) &&

bus/desktop-file.h

@@ -35,6 +35,7 @@
 #define DBUS_SERVICE_EXEC     "Exec"
 #define DBUS_SERVICE_USER     "User"
 #define DBUS_SERVICE_SYSTEMD_SERVICE "SystemdService"
+#define DBUS_SERVICE_ASSUMED_APPARMOR_LABEL "AssumedAppArmorLabel"
 
 typedef struct BusDesktopFile BusDesktopFile;