From dc25979ebb205c4e1596f084fbcd5f78ceca864c Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 21 Nov 2016 20:45:45 +0000 Subject: [PATCH] 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 Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=98666 --- bus/activation.c | 39 +++++++++++++++++++++++++++++++++++++++ bus/activation.h | 1 + bus/apparmor.c | 37 +++++++++++++++++++++++++------------ bus/desktop-file.h | 1 + 4 files changed, 66 insertions(+), 12 deletions(-) diff --git a/bus/activation.c b/bus/activation.c index e131a801e..517af1ec3 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 diff --git a/bus/activation.h b/bus/activation.h index fc5d426f8..7ae8ade9d 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 1701ca4cf..985f5e9fd 100644 --- a/bus/apparmor.c +++ b/bus/apparmor.c @@ -46,6 +46,7 @@ #include #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 e405625c6..144773878 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; -- 2.47.3