Attachment 127877 Details for Bug 98666 – Mediate auto-activation attempts through AppArmor

[patch] Mediate auto-activation attempts through AppArmor

0005-Mediate-auto-activation-attempts-through-AppArmor.patch (text/plain), 10.35 KB, created by Simon McVittie on 2016-11-09 19:45:04 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2016-11-09 19:45:04 UTC

Size: 10.35 KB

patch

obsolete

>From 99998350e3cd1d6247b88f4899586cfec56fc812 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Mon, 7 Nov 2016 19:53:59 +0000
>Subject: [PATCH 5/9] 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.
>---
> bus/activation.c   | 39 +++++++++++++++++++++++++++++++++++++++
> bus/activation.h   |  1 +
> bus/apparmor.c     | 37 +++++++++++++++++++++++++------------
> bus/desktop-file.h |  1 +
> configure.ac       |  2 +-
> 5 files changed, 67 insertions(+), 13 deletions(-)
>
>diff --git a/bus/activation.c b/bus/activation.c
>index e131a80..517af1e 100644
>--- 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 fc5d426..7ae8ade 100644
>--- 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 1701ca4..985f5e9 100644
>--- 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 e405625..1447738 100644
>--- 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;
> 
>diff --git a/configure.ac b/configure.ac
>index 73cecae..cdcad36 100644
>--- a/configure.ac
>+++ b/configure.ac
>@@ -1066,7 +1066,7 @@ fi
> AS_IF([test x$enable_apparmor = xno],
>   [have_apparmor=no],
>   [
>-  PKG_CHECK_MODULES([APPARMOR], [libapparmor >= 2.8.95],
>+  PKG_CHECK_MODULES([APPARMOR], [libapparmor >= 2.10],
>                     [have_apparmor=yes], [have_apparmor=no])
> 
>   AS_IF([test x$enable_apparmor = xauto && test x$have_apparmor = xno],
>-- 
>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.