From 297bcf2fe50978d1281ec9792911b08cd49cd0ea Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 21 Nov 2016 20:45:45 +0000 Subject: [PATCH 07/10] 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 --- 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 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 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 #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; -- 2.10.2