From 9d017bf30fd4d0acbb501f8feded4478e2bc6d70 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 21 Nov 2016 20:56:55 +0000 Subject: [PATCH 06/10] Do not auto-activate services if we could not send a message We specifically do not check recipient policies, because the recipient policy is based on properties of the recipient process (in particular, its uid), which we do not necessarily know until we have already started it. In this initial implementation we do not check LSMs either, because we cannot know what LSM context the recipient process is going to have. However, LSM support will need to be added to make this feature useful, because StartServiceByName is normally allowed in non-LSM environments, and is more powerful than auto-activation anyway. The StartServiceByName method does not go through this check, because if access to that method has been granted, then it's somewhat obvious that you can start arbitrary services. --- bus/activation.c | 22 ++++++++++++++++++++-- bus/apparmor.c | 5 +++++ bus/apparmor.h | 1 + bus/bus.c | 17 +++++++++++++++-- bus/bus.h | 2 ++ bus/connection.c | 3 ++- bus/dispatch.c | 15 +++++++++------ bus/policy.c | 5 +++++ bus/selinux.c | 8 +++++++- bus/selinux.h | 1 + test/sd-activation.c | 38 +++----------------------------------- 11 files changed, 70 insertions(+), 47 deletions(-) diff --git a/bus/activation.c b/bus/activation.c index 1e59190..e131a80 100644 --- a/bus/activation.c +++ b/bus/activation.c @@ -64,7 +64,7 @@ typedef struct DBusHashTable *entries; } BusServiceDirectory; -typedef struct +struct BusActivationEntry { int refcount; char *name; @@ -74,7 +74,7 @@ typedef struct unsigned long mtime; BusServiceDirectory *s_dir; char *filename; -} BusActivationEntry; +}; typedef struct BusPendingActivationEntry BusPendingActivationEntry; @@ -1691,6 +1691,24 @@ bus_activation_activate_service (BusActivation *activation, return FALSE; } + if (auto_activation && + entry != NULL && + !bus_context_check_security_policy (activation->context, + transaction, + connection, /* sender */ + NULL, /* addressed recipient */ + NULL, /* proposed recipient */ + activation_message, + entry, + error)) + { + _DBUS_ASSERT_ERROR_IS_SET (error); + _dbus_verbose ("activation not authorized: %s: %s\n", + error != NULL ? error->name : "(error ignored)", + error != NULL ? error->message : "(error ignored)"); + return FALSE; + } + /* Bypass the registry lookup if we're auto-activating, bus_dispatch would not * call us if the service is already active. */ diff --git a/bus/apparmor.c b/bus/apparmor.c index c679ac5..1701ca4 100644 --- a/bus/apparmor.c +++ b/bus/apparmor.c @@ -739,6 +739,7 @@ bus_apparmor_allows_send (DBusConnection *sender, const char *error_name, const char *destination, const char *source, + BusActivationEntry *activation_entry, DBusError *error) { #ifdef HAVE_APPARMOR @@ -755,6 +756,10 @@ bus_apparmor_allows_send (DBusConnection *sender, 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); diff --git a/bus/apparmor.h b/bus/apparmor.h index 18f3ee7..ed465f7 100644 --- a/bus/apparmor.h +++ b/bus/apparmor.h @@ -56,6 +56,7 @@ dbus_bool_t bus_apparmor_allows_send (DBusConnection *sender, const char *error_name, const char *destination, const char *source, + BusActivationEntry *activation_entry, DBusError *error); dbus_bool_t bus_apparmor_allows_eavesdropping (DBusConnection *connection, diff --git a/bus/bus.c b/bus/bus.c index df8239f..b27b4a5 100644 --- a/bus/bus.c +++ b/bus/bus.c @@ -1511,7 +1511,11 @@ complain_about_message (BusContext *context, * * sender is the sender of the message. * - * NULL for proposed_recipient or sender definitely means the bus driver. + * NULL for sender definitely means the bus driver. + * + * NULL for proposed_recipient may mean the bus driver, or may mean + * we are checking whether service-activation is allowed as a first + * pass before all details of the activated service are known. * * NULL for addressed_recipient may mean the bus driver, or may mean * no destination was specified in the message (e.g. a signal). @@ -1523,6 +1527,7 @@ bus_context_check_security_policy (BusContext *context, DBusConnection *addressed_recipient, DBusConnection *proposed_recipient, DBusMessage *message, + BusActivationEntry *activation_entry, DBusError *error) { const char *src, *dest; @@ -1543,6 +1548,7 @@ bus_context_check_security_policy (BusContext *context, (sender == NULL && !bus_connection_is_active (proposed_recipient))); _dbus_assert (type == DBUS_MESSAGE_TYPE_SIGNAL || addressed_recipient != NULL || + activation_entry != NULL || strcmp (dest, DBUS_SERVICE_DBUS) == 0); switch (type) @@ -1608,7 +1614,9 @@ bus_context_check_security_policy (BusContext *context, dbus_message_get_interface (message), dbus_message_get_member (message), dbus_message_get_error_name (message), - dest ? dest : DBUS_SERVICE_DBUS, error)) + dest ? dest : DBUS_SERVICE_DBUS, + activation_entry, + error)) { if (error != NULL && !dbus_error_is_set (error)) { @@ -1637,6 +1645,7 @@ bus_context_check_security_policy (BusContext *context, dbus_message_get_error_name (message), dest ? dest : DBUS_SERVICE_DBUS, src ? src : DBUS_SERVICE_DBUS, + activation_entry, error)) return FALSE; @@ -1769,6 +1778,10 @@ bus_context_check_security_policy (BusContext *context, /* Record that we will allow a reply here in the future (don't * bother if the recipient is the bus or this is an eavesdropping * connection). Only the addressed recipient may reply. + * + * This isn't done for activation attempts because they have no addressed + * or proposed recipient; when we check whether to actually deliver the + * message, later, we'll record the reply expectation at that point. */ if (type == DBUS_MESSAGE_TYPE_METHOD_CALL && sender && diff --git a/bus/bus.h b/bus/bus.h index ca42533..2e0de82 100644 --- a/bus/bus.h +++ b/bus/bus.h @@ -44,6 +44,7 @@ typedef struct BusOwner BusOwner; typedef struct BusTransaction BusTransaction; typedef struct BusMatchmaker BusMatchmaker; typedef struct BusMatchRule BusMatchRule; +typedef struct BusActivationEntry BusActivationEntry; typedef struct { @@ -141,6 +142,7 @@ dbus_bool_t bus_context_check_security_policy (BusContext DBusConnection *addressed_recipient, DBusConnection *proposed_recipient, DBusMessage *message, + BusActivationEntry *activation_entry, DBusError *error); void bus_context_check_all_watches (BusContext *context); diff --git a/bus/connection.c b/bus/connection.c index 58fc219..4b53cd0 100644 --- a/bus/connection.c +++ b/bus/connection.c @@ -2387,7 +2387,8 @@ bus_transaction_send_from_driver (BusTransaction *transaction, */ if (!bus_context_check_security_policy (bus_transaction_get_context (transaction), transaction, - NULL, connection, connection, message, &error)) + NULL, connection, connection, + message, NULL, &error)) { if (!bus_transaction_capture_error_reply (transaction, connection, &error, message)) diff --git a/bus/dispatch.c b/bus/dispatch.c index 021dfc4..620fd36 100644 --- a/bus/dispatch.c +++ b/bus/dispatch.c @@ -70,6 +70,7 @@ send_one_message (DBusConnection *connection, addressed_recipient, connection, message, + NULL, &stack_error)) { if (!bus_transaction_capture_error_reply (transaction, sender, @@ -147,7 +148,7 @@ bus_dispatch_matches (BusTransaction *transaction, if (!bus_context_check_security_policy (context, transaction, sender, addressed_recipient, addressed_recipient, - message, error)) + message, NULL, error)) return FALSE; if (dbus_message_contains_unix_fds (message) && @@ -380,7 +381,8 @@ bus_dispatch (DBusConnection *connection, } if (!bus_context_check_security_policy (context, transaction, - connection, NULL, NULL, message, &error)) + connection, NULL, NULL, message, + NULL, &error)) { _dbus_verbose ("Security policy rejected message\n"); goto out; @@ -426,12 +428,13 @@ bus_dispatch (DBusConnection *connection, goto out; } - /* We can't do the security policy check here, since the addressed - * recipient service doesn't exist yet. We do it before sending the - * message after the service has been created. - */ activation = bus_connection_get_activation (connection); + /* This will do as much of a security policy check as it can. + * We can't do the full security policy check here, since the + * addressed recipient service doesn't exist yet. We do it before + * sending the message after the service has been created. + */ if (!bus_activation_activate_service (activation, connection, transaction, TRUE, message, service_name, &error)) { diff --git a/bus/policy.c b/bus/policy.c index 082f385..dd0ac86 100644 --- a/bus/policy.c +++ b/bus/policy.c @@ -993,6 +993,11 @@ bus_client_policy_check_can_send (BusClientPolicy *policy, * message bus itself, we check the strings in that case as * built-in services don't have a DBusConnection but messages * to them have a destination service name. + * + * Similarly, receiver can be NULL when we're deciding whether + * activation should be allowed; we make the authorization decision + * on the assumption that the activated service will have the + * requested name and no others. */ if (receiver == NULL) { diff --git a/bus/selinux.c b/bus/selinux.c index 16791c8..e484be6 100644 --- a/bus/selinux.c +++ b/bus/selinux.c @@ -553,6 +553,7 @@ bus_selinux_allows_send (DBusConnection *sender, const char *member, const char *error_name, const char *destination, + BusActivationEntry *activation_entry, DBusError *error) { #ifdef HAVE_SELINUX @@ -566,6 +567,10 @@ bus_selinux_allows_send (DBusConnection *sender, if (!selinux_enabled) return TRUE; + /* We do not mediate activation attempts yet. */ + if (activation_entry) + return TRUE; + if (!sender || !dbus_connection_get_unix_process_id (sender, &spid)) spid = 0; if (!proposed_recipient || !dbus_connection_get_unix_process_id (proposed_recipient, &tpid)) @@ -633,7 +638,8 @@ bus_selinux_allows_send (DBusConnection *sender, } sender_sid = bus_connection_get_selinux_id (sender); - /* A NULL proposed_recipient means the bus itself. */ + + /* A NULL proposed_recipient with no activation entry means the bus itself. */ if (proposed_recipient) recipient_sid = bus_connection_get_selinux_id (proposed_recipient); else diff --git a/bus/selinux.h b/bus/selinux.h index e44c97e..5252b18 100644 --- a/bus/selinux.h +++ b/bus/selinux.h @@ -61,6 +61,7 @@ dbus_bool_t bus_selinux_allows_send (DBusConnection *sender, const char *member, const char *error_name, const char *destination, + BusActivationEntry *activation_entry, DBusError *error); BusSELinuxID* bus_selinux_init_connection_id (DBusConnection *connection, diff --git a/test/sd-activation.c b/test/sd-activation.c index 6d52987..9b2a5bb 100644 --- a/test/sd-activation.c +++ b/test/sd-activation.c @@ -575,44 +575,12 @@ test_deny_send (Fixture *f, dbus_connection_send (f->caller, m, NULL); dbus_message_unref (m); - /* The fake systemd connects to the bus. */ - f->systemd = test_connect_to_bus (f->ctx, f->address); - if (!dbus_connection_add_filter (f->systemd, systemd_filter, f, NULL)) - g_error ("OOM"); - f->systemd_filter_added = TRUE; - f->systemd_name = dbus_bus_get_unique_name (f->systemd); - take_well_known_name (f, f->systemd, "org.freedesktop.systemd1"); - - /* It gets its activation request. */ - while (f->caller_message == NULL && f->systemd_message == NULL) - test_main_context_iterate (f->ctx, TRUE); - - g_assert (f->caller_message == NULL); - g_assert (f->systemd_message != NULL); - - m = f->systemd_message; - f->systemd_message = NULL; - assert_signal (m, DBUS_SERVICE_DBUS, DBUS_PATH_DBUS, - "org.freedesktop.systemd1.Activator", "ActivationRequest", "s", - "org.freedesktop.systemd1"); - dbus_message_unref (m); - - /* systemd starts the activatable service. */ - f->activated = test_connect_to_bus (f->ctx, f->address); - if (!dbus_connection_add_filter (f->activated, activated_filter, - f, NULL)) - g_error ("OOM"); - f->activated_filter_added = TRUE; - f->activated_name = dbus_bus_get_unique_name (f->activated); - take_well_known_name (f, f->activated, "com.example.SendDenied"); + /* Even before the fake systemd connects to the bus, we get an error + * back: activation is not allowed. */ - /* We re-do the message matching, and now the message is - * forbidden by the receive policy. */ - while (f->activated_message == NULL && f->caller_message == NULL) + while (f->caller_message == NULL) test_main_context_iterate (f->ctx, TRUE); - g_assert (f->activated_message == NULL); - m = f->caller_message; f->caller_message = NULL; assert_error_reply (m, DBUS_SERVICE_DBUS, f->caller_name, -- 2.10.2