From a2b0e7b669798f8ea546e89dc8948502f07b88b7 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 17 Apr 2018 12:38:00 +0100 Subject: [PATCH 12/39] containers: Add stub functions to check against filtering Signed-off-by: Simon McVittie --- bus/activation.c | 22 ++++++- bus/bus.c | 15 +++++ bus/containers.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++- bus/containers.h | 25 ++++++++ bus/dispatch.c | 41 ++++++++++++- bus/dispatch.h | 2 + bus/driver.c | 96 +++++++++++++++++++++++++++++- bus/services.c | 36 +++++++++++- 8 files changed, 375 insertions(+), 10 deletions(-) diff --git a/bus/activation.c b/bus/activation.c index 4e785319..67067c13 100644 --- a/bus/activation.c +++ b/bus/activation.c @@ -27,6 +27,7 @@ #include "activation.h" #include "activation-exit-codes.h" #include "config-parser.h" +#include "containers.h" #include "desktop-file.h" #include "dispatch.h" #include "services.h" @@ -1310,7 +1311,8 @@ bus_activation_send_pending_auto_activation_messages (BusActivation *activation if (!bus_dispatch_matches (transaction, entry->connection, addressed_recipient, - entry->activation_message, &error)) + entry->activation_message, + NULL, NULL, &error)) { /* If permission is denied, we just want to return the error * to the original method invoker; in particular, we don't @@ -1837,6 +1839,10 @@ bus_activation_activate_service (BusActivation *activation, limit = bus_context_get_max_pending_activations (activation->context); + if (connection != NULL && + !bus_containers_check_can_activate (connection, service_name, error)) + return FALSE; + if (activation->n_pending_activations >= limit) { dbus_set_error (error, DBUS_ERROR_LIMITS_EXCEEDED, @@ -2160,7 +2166,8 @@ bus_activation_activate_service (BusActivation *activation, bus_connection_get_loginfo (connection)); /* Wonderful, systemd is connected, let's just send the msg */ retval = bus_dispatch_matches (activation_transaction, NULL, - systemd, message, error); + systemd, message, + NULL, NULL, error); } else { @@ -2375,6 +2382,15 @@ bus_activation_list_services (BusActivation *activation, { BusActivationEntry *entry = _dbus_hash_iter_get_value (&iter); + /* Unique names aren't activatable */ + _dbus_assert (entry->name[0] != ':'); + + /* Skip the ones the observer is not allowed to know about */ + if (observer != NULL && + !bus_containers_check_can_see_well_known_name (observer, + entry->name)) + continue; + retval[i] = _dbus_strdup (entry->name); if (retval[i] == NULL) goto error; @@ -2385,7 +2401,7 @@ bus_activation_list_services (BusActivation *activation, retval[i] = NULL; if (array_len) - *array_len = len; + *array_len = i; *listp = retval; return TRUE; diff --git a/bus/bus.c b/bus/bus.c index 9fd9820b..d0cba768 100644 --- a/bus/bus.c +++ b/bus/bus.c @@ -1674,6 +1674,12 @@ bus_context_check_security_policy (BusContext *context, sender_policy = NULL; } + if (!bus_containers_check_can_send (sender, + requested_reply, + proposed_recipient, + message, error)) + return FALSE; + /* First verify the SELinux access controls. If allowed then * go on with the standard checks. */ @@ -1788,6 +1794,15 @@ bus_context_check_security_policy (BusContext *context, (proposed_recipient != NULL && sender == NULL && recipient_policy == NULL) || (proposed_recipient == NULL && recipient_policy == NULL)); + if (proposed_recipient != NULL && + !bus_containers_check_can_receive (sender, + requested_reply, + proposed_recipient, + addressed_recipient, + message, + error)) + return FALSE; + log = FALSE; if (sender_policy && !bus_client_policy_check_can_send (sender_policy, diff --git a/bus/containers.c b/bus/containers.c index ece12eab..5386cb99 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -238,7 +238,8 @@ bus_container_instance_emit_removed (BusContainerInstance *self) !bus_transaction_capture (transaction, NULL, NULL, message)) goto oom; - if (!bus_dispatch_matches (transaction, NULL, NULL, message, &error)) + if (!bus_dispatch_matches (transaction, NULL, NULL, message, + NULL, NULL, &error)) { if (dbus_error_has_name (&error, DBUS_ERROR_NO_MEMORY)) goto oom; @@ -1474,3 +1475,148 @@ bus_containers_connection_is_contained (DBusConnection *connection, return FALSE; } + +/* + * Return TRUE if caller can cause activation of name. + */ +dbus_bool_t +bus_containers_check_can_activate (DBusConnection *caller, + const char *name, + DBusError *error) +{ + _dbus_assert (caller != NULL); + _dbus_assert (name != NULL); + _DBUS_ASSERT_ERROR_IS_CLEAR (error); + + return TRUE; +} + +/* + * Return TRUE if connection is allowed to own bus_name. + */ +dbus_bool_t +bus_containers_check_can_own (DBusConnection *connection, + const char *bus_name, + DBusError *error) +{ + _dbus_assert (connection != NULL); + _dbus_assert (bus_name != NULL); + _DBUS_ASSERT_ERROR_IS_CLEAR (error); + + return TRUE; +} + +/* + * Return TRUE if sender is allowed to send message to + * proposed_recipient. + * + * sender is always non-NULL here. + * + * proposed_recipient may be NULL if the message is to be sent to the + * bus driver, or if we are checking whether auto-starting is allowed + * (as a first pass, before all details of the activated service + * are known). This is the same as bus_context_check_security_policy(). + * For broadcasts, this function is called once per recipient. + * + * requested_reply indicates whether it's an expected reply to a method + * call from proposed_recipient to sender that was already allowed. + */ +dbus_bool_t +bus_containers_check_can_send (DBusConnection *sender, + dbus_bool_t requested_reply, + DBusConnection *proposed_recipient, + DBusMessage *message, + DBusError *error) +{ + _dbus_assert (sender != NULL); + _dbus_assert (message != NULL); + _DBUS_ASSERT_ERROR_IS_CLEAR (error); + + return TRUE; +} + +/* + * Return TRUE if proposed_recipient can receive message from sender. + * + * sender may be NULL to indicate the dbus-daemon. + * + * requested_reply is the same as for check_can_send(). + * + * proposed_recipient is the recipient we are considering sending to + * right now, which may be an eavesdropper but is never NULL. + * For broadcasts, this function is called once per recipient. + * + * addressed_recipient is proposed_recipient, or NULL for broadcasts, + * or a different connection if the proposed recipient is + * eavesdropping. + */ +dbus_bool_t +bus_containers_check_can_receive (DBusConnection *sender, + dbus_bool_t requested_reply, + DBusConnection *proposed_recipient, + DBusConnection *addressed_recipient, + DBusMessage *message, + DBusError *error) +{ + _dbus_assert (proposed_recipient != NULL); + _dbus_assert (message != NULL); + _DBUS_ASSERT_ERROR_IS_CLEAR (error); + + return TRUE; +} + +/* + * Return TRUE if observer can see name. + * + * This doesn't have a DBusError, because on error we must always + * behave as though the name didn't exist, to avoid leaking + * information. + */ +dbus_bool_t +bus_containers_check_can_see_well_known_name (DBusConnection *observer, + const char *name) +{ + _dbus_assert (observer != NULL); + _dbus_assert (name != NULL); + _dbus_assert (name[0] != ':'); + + return TRUE; +} + +/* + * Return TRUE if observer can see subject. + * + * This doesn't have a DBusError, because on error we must always + * behave as though the name didn't exist, to avoid leaking + * information. + */ +dbus_bool_t +bus_containers_check_can_see_connection (DBusConnection *observer, + DBusConnection *subject) +{ + _dbus_assert (observer != NULL); + _dbus_assert (subject != NULL); + + return TRUE; +} + +/* + * Return TRUE if observer can retrieve various sorts of information + * (credentials etc.) about other. other may be NULL to denote the + * dbus-daemon itself. + * + * To avoid leaking information about the existence or nonexistence of + * names, this should not be called unless one of the + * bus_containers_check_can_see_*() functions has already succeeded for + * the name or connection in question. + */ +dbus_bool_t +bus_containers_check_can_inspect (DBusConnection *observer, + DBusConnection *other, + DBusError *error) +{ + _dbus_assert (observer != NULL); + _DBUS_ASSERT_ERROR_IS_CLEAR (error); + + return TRUE; +} diff --git a/bus/containers.h b/bus/containers.h index 7046bcbc..0d04c702 100644 --- a/bus/containers.h +++ b/bus/containers.h @@ -66,6 +66,31 @@ dbus_bool_t bus_containers_connection_is_contained (DBusConnection *connectio const char **type, const char **name); +dbus_bool_t bus_containers_check_can_activate (DBusConnection *caller, + const char *name, + DBusError *error); +dbus_bool_t bus_containers_check_can_inspect (DBusConnection *observer, + DBusConnection *other, + DBusError *error); +dbus_bool_t bus_containers_check_can_own (DBusConnection *connection, + const char *bus_name, + DBusError *error); +dbus_bool_t bus_containers_check_can_receive (DBusConnection *sender, + dbus_bool_t requested_reply, + DBusConnection *proposed_recipient, + DBusConnection *addressed_recipient, + DBusMessage *message, + DBusError *error); +dbus_bool_t bus_containers_check_can_see_connection (DBusConnection *observer, + DBusConnection *subject); +dbus_bool_t bus_containers_check_can_see_well_known_name (DBusConnection *observer, + const char *name); +dbus_bool_t bus_containers_check_can_send (DBusConnection *sender, + dbus_bool_t requested_reply, + DBusConnection *proposed_recipient, + DBusMessage *message, + DBusError *error); + static inline void bus_clear_containers (BusContainers **containers_p) { diff --git a/bus/dispatch.c b/bus/dispatch.c index 3c4c031b..4ed5d0bd 100644 --- a/bus/dispatch.c +++ b/bus/dispatch.c @@ -26,6 +26,7 @@ #include #include "dispatch.h" #include "connection.h" +#include "containers.h" #include "driver.h" #include "services.h" #include "activation.h" @@ -63,10 +64,32 @@ send_one_message (DBusConnection *connection, DBusConnection *addressed_recipient, DBusMessage *message, BusTransaction *transaction, + DBusConnection *needs_to_see_conn, + const char *needs_to_see_well_known_name, DBusError *error) { DBusError stack_error = DBUS_ERROR_INIT; + if (needs_to_see_conn != NULL && + !bus_containers_check_can_see_connection (connection, + needs_to_see_conn)) + { + /* This broadcast is about a connection that is invisible to this + * recipient. Don't send it, but don't set or return an + * error either. */ + return TRUE; + } + + if (needs_to_see_well_known_name != NULL && + !bus_containers_check_can_see_well_known_name (connection, + needs_to_see_well_known_name)) + { + /* This broadcast is about a well-known name that is invisible to this + * recipient. Don't send it, but don't set or return an + * error either. */ + return TRUE; + } + if (!bus_context_check_security_policy (context, transaction, sender, addressed_recipient, @@ -122,6 +145,8 @@ bus_dispatch_matches (BusTransaction *transaction, DBusConnection *sender, DBusConnection *addressed_recipient, DBusMessage *message, + DBusConnection *needs_to_see_conn, + const char *needs_to_see_well_known_name, DBusError *error) { DBusError tmp_error; @@ -131,6 +156,11 @@ bus_dispatch_matches (BusTransaction *transaction, DBusList *link; BusContext *context; + /* needs_to_see is only for broadcasts */ + _dbus_assert (addressed_recipient == NULL || needs_to_see_conn == NULL); + _dbus_assert (addressed_recipient == NULL || + needs_to_see_well_known_name == NULL); + _DBUS_ASSERT_ERROR_IS_CLEAR (error); /* sender and recipient can both be NULL for the bus driver, @@ -145,6 +175,11 @@ bus_dispatch_matches (BusTransaction *transaction, /* First, send the message to the addressed_recipient, if there is one. */ if (addressed_recipient != NULL) { + /* We don't need to check these because they only make sense for + * broadcasts */ + _dbus_assert (needs_to_see_conn == NULL); + _dbus_assert (needs_to_see_well_known_name == NULL); + if (!bus_context_check_security_policy (context, transaction, sender, addressed_recipient, addressed_recipient, @@ -193,7 +228,8 @@ bus_dispatch_matches (BusTransaction *transaction, dest = link->data; if (!send_one_message (dest, context, sender, addressed_recipient, - message, transaction, &tmp_error)) + message, transaction, needs_to_see_conn, + needs_to_see_well_known_name, &tmp_error)) break; link = _dbus_list_get_next_link (&recipients, link); @@ -497,7 +533,8 @@ bus_dispatch (DBusConnection *connection, * addressed_recipient == NULL), and match it against other connections' * match rules. */ - if (!bus_dispatch_matches (transaction, connection, addressed_recipient, message, &error)) + if (!bus_dispatch_matches (transaction, connection, addressed_recipient, + message, NULL, NULL, &error)) goto out; out: diff --git a/bus/dispatch.h b/bus/dispatch.h index fb5ba7a5..d93d7133 100644 --- a/bus/dispatch.h +++ b/bus/dispatch.h @@ -33,6 +33,8 @@ dbus_bool_t bus_dispatch_matches (BusTransaction *transaction, DBusConnection *sender, DBusConnection *recipient, DBusMessage *message, + DBusConnection *needs_to_see_conn, + const char *needs_to_see_well_known_name, DBusError *error); #endif /* BUS_DISPATCH_H */ diff --git a/bus/driver.c b/bus/driver.c index 87641b1f..7ad9d49b 100644 --- a/bus/driver.c +++ b/bus/driver.c @@ -58,6 +58,7 @@ bus_driver_get_owner_of_name (DBusConnection *observer, BusRegistry *registry; BusService *serv; DBusString str; + DBusConnection *owner; registry = bus_connection_get_registry (observer); _dbus_string_init_const (&str, name); @@ -66,7 +67,21 @@ bus_driver_get_owner_of_name (DBusConnection *observer, if (serv == NULL) return NULL; - return bus_service_get_primary_owners_connection (serv); + owner = bus_service_get_primary_owners_connection (serv); + + if (name[0] == ':') + { + if (owner != NULL && + !bus_containers_check_can_see_connection (observer, owner)) + return NULL; + } + else + { + if (!bus_containers_check_can_see_well_known_name (observer, name)) + return NULL; + } + + return owner; } BusDriverFound @@ -92,7 +107,12 @@ bus_driver_get_conn_helper (DBusConnection *connection, *name_p = name; if (strcmp (name, DBUS_SERVICE_DBUS) == 0) - return BUS_DRIVER_FOUND_SELF; + { + if (!bus_containers_check_can_inspect (connection, NULL, error)) + return BUS_DRIVER_FOUND_ERROR; + + return BUS_DRIVER_FOUND_SELF; + } conn = bus_driver_get_owner_of_name (connection, name); @@ -104,6 +124,9 @@ bus_driver_get_conn_helper (DBusConnection *connection, return BUS_DRIVER_FOUND_ERROR; } + if (!bus_containers_check_can_inspect (connection, conn, error)) + return BUS_DRIVER_FOUND_ERROR; + if (peer_conn_p != NULL) *peer_conn_p = conn; @@ -247,6 +270,8 @@ bus_driver_send_name_owner_changed (const char *service_name, { const char *old_owner_name = ""; const char *new_owner_name = ""; + DBusConnection *needs_to_see_conn; + const char *needs_to_see_well_known_name; DBusMessage *message; dbus_bool_t retval; @@ -288,7 +313,44 @@ bus_driver_send_name_owner_changed (const char *service_name, if (!bus_transaction_capture (transaction, NULL, NULL, message)) goto oom; - retval = bus_dispatch_matches (transaction, NULL, NULL, message, error); + if (service_name[0] == ':') + { + /* For the creation or destruction of a unique name, we should only + * send the message to connections that are allowed to see the + * relevant connection, and it's easier to implement that in terms + * of a DBusConnection. + * + * We know that unique names are only created or destroyed, never + * atomically transferred between owners. */ + + if (old_owner != NULL) + { + _dbus_assert (strcmp (service_name, old_owner_name) == 0); + _dbus_assert (strcmp (new_owner_name, "") == 0); + _dbus_assert (new_owner == NULL); + needs_to_see_conn = old_owner; + } + else + { + _dbus_assert (new_owner != NULL); + _dbus_assert (strcmp (old_owner_name, "") == 0); + _dbus_assert (strcmp (service_name, new_owner_name) == 0); + needs_to_see_conn = new_owner; + } + + needs_to_see_well_known_name = NULL; + } + else + { + /* We filter NameOwnerChanged messages for well-known names based + * on text matching. */ + needs_to_see_conn = NULL; + needs_to_see_well_known_name = service_name; + } + + retval = bus_dispatch_matches (transaction, NULL, NULL, message, + needs_to_see_conn, + needs_to_see_well_known_name, error); dbus_message_unref (message); return retval; @@ -923,6 +985,11 @@ bus_driver_handle_service_exists (DBusConnection *connection, { service_exists = TRUE; } + else if (name[0] != ':' && + !bus_containers_check_can_see_well_known_name (connection, name)) + { + service_exists = FALSE; + } else { _dbus_string_init_const (&service_name, name); @@ -930,6 +997,16 @@ bus_driver_handle_service_exists (DBusConnection *connection, service_exists = service != NULL; } + if (service_exists && name[0] == ':') + { + DBusConnection *owner; + + owner = bus_service_get_primary_owners_connection (service); + + if (!bus_containers_check_can_see_connection (connection, owner)) + service_exists = FALSE; + } + reply = dbus_message_new_method_return (message); if (reply == NULL) { @@ -1502,6 +1579,10 @@ bus_driver_handle_get_service_owner (DBusConnection *connection, goto failed; } + if (text[0] != ':' && + !bus_containers_check_can_see_well_known_name (connection, text)) + goto invisible; + service = bus_registry_lookup (registry, &str); if (service == NULL) @@ -1509,6 +1590,10 @@ bus_driver_handle_get_service_owner (DBusConnection *connection, owner = bus_service_get_primary_owners_connection (service); + if (text[0] == ':' && + !bus_containers_check_can_see_connection (connection, owner)) + goto invisible; + base_name = bus_connection_get_name (owner); if (base_name == NULL) { @@ -1597,6 +1682,11 @@ bus_driver_handle_list_queued_owners (DBusConnection *connection, goto success; } + /* TODO: In flatpak-dbus-proxy, ListQueuedOwners() is mediated by + * OWN access, but SEE access might make more sense? */ + if (!bus_containers_check_can_own (connection, text, NULL)) + goto invisible; + _dbus_string_init_const (&str, text); service = bus_registry_lookup (registry, &str); diff --git a/bus/services.c b/bus/services.c index c1e645ed..7dc6fb33 100644 --- a/bus/services.c +++ b/bus/services.c @@ -28,6 +28,7 @@ #include #include +#include "containers.h" #include "driver.h" #include "services.h" #include "connection.h" @@ -354,6 +355,26 @@ bus_registry_list_services (BusRegistry *registry, { BusService *service = _dbus_hash_iter_get_value (&iter); + /* Skip the ones the observer is not allowed to know about */ + if (observer != NULL) + { + + if (service->name[0] == ':') + { + DBusConnection *owner; + + owner = bus_service_get_primary_owners_connection (service); + + if (!bus_containers_check_can_see_connection (observer, owner)) + continue; + } + else if (!bus_containers_check_can_see_well_known_name (observer, + service->name)) + { + continue; + } + } + retval[i] = _dbus_strdup (service->name); if (retval[i] == NULL) goto error; @@ -364,7 +385,7 @@ bus_registry_list_services (BusRegistry *registry, retval[i] = NULL; if (array_len) - *array_len = len; + *array_len = i; *listp = retval; return TRUE; @@ -434,6 +455,11 @@ bus_registry_acquire_service (BusRegistry *registry, goto out; } + if (!bus_containers_check_can_own (connection, + _dbus_string_get_const_data (service_name), + error)) + goto out; + policy = bus_connection_get_policy (connection); _dbus_assert (policy != NULL); @@ -665,6 +691,14 @@ bus_registry_release_service (BusRegistry *registry, goto out; } + /* Check whether we would be allowed to own the name before we try + * releasing it, so that contained apps can't use the reply from this + * method as an oracle to determine whether someone else owns it. */ + if (!bus_containers_check_can_own (connection, + _dbus_string_get_const_data (service_name), + error)) + goto out; + service = bus_registry_lookup (registry, service_name); if (service == NULL) -- 2.17.0