From 535ed8b1cf2e08b33101de05fbed5397570b8076 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 11 May 2018 18:22:51 +0100 Subject: [PATCH 13/39] bus: If containers block a requested reply, substitute an error reply If some connection in a container has legitimately had one of its methods called by another connection (either unconfined or in a different container), but the reply is unsuitably for delivery to the caller (perhaps because it contains fds), we don't want the caller to be left waiting. Add a way for the security policy to request that we send an error reply to the original method call, as well as sending an error reply to the undesirable reply. Signed-off-by: Simon McVittie --- bus/activation.c | 1 + bus/bus.c | 17 ++++++++++--- bus/bus.h | 1 + bus/connection.c | 2 +- bus/dispatch.c | 62 ++++++++++++++++++++++++++++++++++++++++++++---- 5 files changed, 74 insertions(+), 9 deletions(-) diff --git a/bus/activation.c b/bus/activation.c index 67067c13..715f6371 100644 --- a/bus/activation.c +++ b/bus/activation.c @@ -1875,6 +1875,7 @@ bus_activation_activate_service (BusActivation *activation, NULL, /* addressed recipient */ NULL, /* proposed recipient */ activation_message, + NULL, entry, error)) { diff --git a/bus/bus.c b/bus/bus.c index d0cba768..e5dc6d88 100644 --- a/bus/bus.c +++ b/bus/bus.c @@ -1587,6 +1587,11 @@ complain_about_message (BusContext *context, * * NULL for addressed_recipient may mean the bus driver, or may mean * no destination was specified in the message (e.g. a signal). + * + * If send_error_reply is non-NULL, set it to a message serial number + * if it is appropriate to send error to the addressed recipient (as + * a reply to the given serial number) in addition to the sender + * (as reply to message). */ dbus_bool_t bus_context_check_security_policy (BusContext *context, @@ -1595,6 +1600,7 @@ bus_context_check_security_policy (BusContext *context, DBusConnection *addressed_recipient, DBusConnection *proposed_recipient, DBusMessage *message, + dbus_uint32_t *send_error_reply, BusActivationEntry *activation_entry, DBusError *error) { @@ -1605,6 +1611,7 @@ bus_context_check_security_policy (BusContext *context, dbus_bool_t log; int type; dbus_bool_t requested_reply; + DBusError error2 = DBUS_ERROR_INIT; type = dbus_message_get_type (message); src = dbus_message_get_sender (message); @@ -1618,6 +1625,7 @@ bus_context_check_security_policy (BusContext *context, addressed_recipient != NULL || activation_entry != NULL || strcmp (dest, DBUS_SERVICE_DBUS) == 0); + _dbus_assert (send_error_reply == NULL || *send_error_reply == 0); switch (type) { @@ -1654,8 +1662,6 @@ bus_context_check_security_policy (BusContext *context, if (proposed_recipient != NULL /* not to the bus driver */ && addressed_recipient == proposed_recipient /* not eavesdropping */) { - DBusError error2; - dbus_error_init (&error2); requested_reply = bus_connections_check_reply (bus_connection_get_connections (sender), transaction, @@ -1678,7 +1684,12 @@ bus_context_check_security_policy (BusContext *context, requested_reply, proposed_recipient, message, error)) - return FALSE; + { + if (requested_reply && send_error_reply != NULL) + *send_error_reply = dbus_message_get_reply_serial (message); + + return FALSE; + } /* First verify the SELinux access controls. If allowed then * go on with the standard checks. diff --git a/bus/bus.h b/bus/bus.h index 8f96222f..5fcecc6e 100644 --- a/bus/bus.h +++ b/bus/bus.h @@ -152,6 +152,7 @@ dbus_bool_t bus_context_check_security_policy (BusContext DBusConnection *addressed_recipient, DBusConnection *proposed_recipient, DBusMessage *message, + dbus_uint32_t *send_error_reply, BusActivationEntry *activation_entry, DBusError *error); void bus_context_check_all_watches (BusContext *context); diff --git a/bus/connection.c b/bus/connection.c index 35f8e36e..f8e69850 100644 --- a/bus/connection.c +++ b/bus/connection.c @@ -2443,7 +2443,7 @@ bus_transaction_send_from_driver (BusTransaction *transaction, if (!bus_context_check_security_policy (bus_transaction_get_context (transaction), transaction, NULL, connection, connection, - message, NULL, &error)) + message, NULL, NULL, &error)) { if (!bus_transaction_capture_error_reply (transaction, connection, &error, message)) diff --git a/bus/dispatch.c b/bus/dispatch.c index 4ed5d0bd..1c99298e 100644 --- a/bus/dispatch.c +++ b/bus/dispatch.c @@ -96,6 +96,7 @@ send_one_message (DBusConnection *connection, connection, message, NULL, + NULL, &stack_error)) { if (!bus_transaction_capture_error_reply (transaction, sender, @@ -149,7 +150,7 @@ bus_dispatch_matches (BusTransaction *transaction, const char *needs_to_see_well_known_name, DBusError *error) { - DBusError tmp_error; + DBusError tmp_error = DBUS_ERROR_INIT; BusConnections *connections; DBusList *recipients; BusMatchmaker *matchmaker; @@ -175,6 +176,8 @@ bus_dispatch_matches (BusTransaction *transaction, /* First, send the message to the addressed_recipient, if there is one. */ if (addressed_recipient != NULL) { + dbus_uint32_t send_error_reply = 0; + /* We don't need to check these because they only make sense for * broadcasts */ _dbus_assert (needs_to_see_conn == NULL); @@ -183,8 +186,58 @@ bus_dispatch_matches (BusTransaction *transaction, if (!bus_context_check_security_policy (context, transaction, sender, addressed_recipient, addressed_recipient, - message, NULL, error)) - return FALSE; + message, &send_error_reply, + NULL, &tmp_error)) + { + /* If the security policy told us to send an error reply to + * the addressed recipient instead, do so */ + if (send_error_reply != 0) + { + DBusMessage *substitute_message; + + substitute_message = dbus_message_new (DBUS_MESSAGE_TYPE_ERROR); + + if (substitute_message == NULL || + !dbus_message_set_reply_serial (substitute_message, + send_error_reply) || + !dbus_message_set_sender (substitute_message, + bus_connection_get_name (sender)) || + !dbus_message_set_destination ( + substitute_message, + bus_connection_get_name (addressed_recipient)) || + !dbus_message_set_error_name (substitute_message, + tmp_error.name) || + !dbus_message_append_args (substitute_message, + DBUS_TYPE_STRING, &tmp_error.message, + DBUS_TYPE_INVALID)) + { + dbus_clear_message (&substitute_message); + dbus_error_free (&tmp_error); + BUS_SET_OOM (error); + return FALSE; + } + + dbus_message_set_no_reply (substitute_message, TRUE); + + if (!bus_transaction_capture (transaction, NULL, + addressed_recipient, + substitute_message) || + !bus_transaction_send (transaction, sender, + addressed_recipient, + substitute_message)) + { + dbus_clear_message (&substitute_message); + dbus_error_free (&tmp_error); + BUS_SET_OOM (error); + return FALSE; + } + + dbus_message_unref (substitute_message); + } + + dbus_move_error (&tmp_error, error); + return FALSE; + } if (dbus_message_contains_unix_fds (message) && !dbus_connection_can_send_type (addressed_recipient, @@ -208,7 +261,6 @@ bus_dispatch_matches (BusTransaction *transaction, /* Now dispatch to others who look interested in this message */ connections = bus_context_get_connections (context); - dbus_error_init (&tmp_error); matchmaker = bus_context_get_matchmaker (context); recipients = NULL; @@ -429,7 +481,7 @@ bus_dispatch (DBusConnection *connection, if (!bus_context_check_security_policy (context, transaction, connection, NULL, NULL, message, - NULL, &error)) + NULL, NULL, &error)) { _dbus_verbose ("Security policy rejected message\n"); goto out; -- 2.17.0