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

diff --git a/bus/activation.c b/bus/activation.c
index e131a801e6eb9f2c2867a1ee7bdbe4aca2609606..517af1ec30b6c00ebb5cdcd2ff8f3c468644dbf4 100644 (file)
--- a/bus/activation.c
+++ b/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>

diff --git a/bus/activation.h b/bus/activation.h
index fc5d426f84a4076dc76ac71f6aa9540fc5ecd413..7ae8ade9dacc63b4372221b8fa764516624f0da9 100644 (file)
--- a/bus/activation.h
+++ b/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 */

diff --git a/bus/apparmor.c b/bus/apparmor.c
index 1701ca4cfbed51105bffc745eac3cd98f882a53d..985f5e9fd3e9af167fd94fec7d5a17dcea004736 100644 (file)
--- a/bus/apparmor.c
+++ b/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) &&

diff --git a/bus/desktop-file.h b/bus/desktop-file.h
index e405625c6bbc22e167ec3a17d551a5e79a033052..14477387827e57aa0ed7724f21c1d173e2e7b9b0 100644 (file)
--- a/bus/desktop-file.h
+++ b/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;