From a860b0d7ad8e93274f7bea3a3466b8a91e9bada1 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 31 May 2017 18:27:11 +0100 Subject: [PATCH 3/4] bus/driver: Only allow specific methods to be called at wrong paths The default for the future should be that new functionality should only be available at /org/freedesktop/DBus, which is the canonical path of the "bus driver" object that represents the message bus. The disallowed methods are still in Introspect() output, and raise AccessDenied instead of UnknownMethod, to preserve the invariant that the o.fd.DBus interface has constant contents. test/dbus-daemon.c already asserts that UpdateActivationEnvironment() still raises AccessDenied when invoked on a non-canonical path; this has been in place since 1.8.14. Signed-off-by: Simon McVittie --- bus/driver.c | 133 ++++++++++++++++++++++++++++++++--------------------------- bus/driver.h | 2 - 2 files changed, 72 insertions(+), 63 deletions(-) diff --git a/bus/driver.c b/bus/driver.c index 25b77f99..c150e3e1 100644 --- a/bus/driver.c +++ b/bus/driver.c @@ -1090,9 +1090,6 @@ bus_driver_handle_update_activation_environment (DBusConnection *connection, _DBUS_ASSERT_ERROR_IS_CLEAR (error); - if (!bus_driver_check_message_is_for_us (message, error)) - return FALSE; - #ifdef DBUS_UNIX { /* UpdateActivationEnvironment is basically a recipe for privilege @@ -2292,6 +2289,15 @@ out: return ret; } +typedef enum +{ + METHOD_FLAG_NONE = 0, + /* Various older methods were available at every object path. We have to + * preserve that behaviour for backwards compatibility, but we can at least + * stop doing that for newly added methods.. */ + METHOD_FLAG_ANY_PATH = (1 << 0) +} MethodFlags; + typedef struct { const char *name; @@ -2301,6 +2307,7 @@ typedef struct BusTransaction *transaction, DBusMessage *message, DBusError *error); + MethodFlags flags; } MessageHandler; /* For speed it might be useful to sort this in order of @@ -2311,77 +2318,96 @@ static const MessageHandler dbus_message_handlers[] = { { "Hello", "", DBUS_TYPE_STRING_AS_STRING, - bus_driver_handle_hello }, + bus_driver_handle_hello, + METHOD_FLAG_ANY_PATH }, { "RequestName", DBUS_TYPE_STRING_AS_STRING DBUS_TYPE_UINT32_AS_STRING, DBUS_TYPE_UINT32_AS_STRING, - bus_driver_handle_acquire_service }, + bus_driver_handle_acquire_service, + METHOD_FLAG_ANY_PATH }, { "ReleaseName", DBUS_TYPE_STRING_AS_STRING, DBUS_TYPE_UINT32_AS_STRING, - bus_driver_handle_release_service }, + bus_driver_handle_release_service, + METHOD_FLAG_ANY_PATH }, { "StartServiceByName", DBUS_TYPE_STRING_AS_STRING DBUS_TYPE_UINT32_AS_STRING, DBUS_TYPE_UINT32_AS_STRING, - bus_driver_handle_activate_service }, + bus_driver_handle_activate_service, + METHOD_FLAG_ANY_PATH }, { "UpdateActivationEnvironment", DBUS_TYPE_ARRAY_AS_STRING DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING DBUS_TYPE_STRING_AS_STRING DBUS_TYPE_STRING_AS_STRING DBUS_DICT_ENTRY_END_CHAR_AS_STRING, "", - bus_driver_handle_update_activation_environment }, + bus_driver_handle_update_activation_environment, + METHOD_FLAG_NONE }, { "NameHasOwner", DBUS_TYPE_STRING_AS_STRING, DBUS_TYPE_BOOLEAN_AS_STRING, - bus_driver_handle_service_exists }, + bus_driver_handle_service_exists, + METHOD_FLAG_ANY_PATH }, { "ListNames", "", DBUS_TYPE_ARRAY_AS_STRING DBUS_TYPE_STRING_AS_STRING, - bus_driver_handle_list_services }, + bus_driver_handle_list_services, + METHOD_FLAG_ANY_PATH }, { "ListActivatableNames", "", DBUS_TYPE_ARRAY_AS_STRING DBUS_TYPE_STRING_AS_STRING, - bus_driver_handle_list_activatable_services }, + bus_driver_handle_list_activatable_services, + METHOD_FLAG_ANY_PATH }, { "AddMatch", DBUS_TYPE_STRING_AS_STRING, "", - bus_driver_handle_add_match }, + bus_driver_handle_add_match, + METHOD_FLAG_ANY_PATH }, { "RemoveMatch", DBUS_TYPE_STRING_AS_STRING, "", - bus_driver_handle_remove_match }, + bus_driver_handle_remove_match, + METHOD_FLAG_ANY_PATH }, { "GetNameOwner", DBUS_TYPE_STRING_AS_STRING, DBUS_TYPE_STRING_AS_STRING, - bus_driver_handle_get_service_owner }, + bus_driver_handle_get_service_owner, + METHOD_FLAG_ANY_PATH }, { "ListQueuedOwners", DBUS_TYPE_STRING_AS_STRING, DBUS_TYPE_ARRAY_AS_STRING DBUS_TYPE_STRING_AS_STRING, - bus_driver_handle_list_queued_owners }, + bus_driver_handle_list_queued_owners, + METHOD_FLAG_ANY_PATH }, { "GetConnectionUnixUser", DBUS_TYPE_STRING_AS_STRING, DBUS_TYPE_UINT32_AS_STRING, - bus_driver_handle_get_connection_unix_user }, + bus_driver_handle_get_connection_unix_user, + METHOD_FLAG_ANY_PATH }, { "GetConnectionUnixProcessID", DBUS_TYPE_STRING_AS_STRING, DBUS_TYPE_UINT32_AS_STRING, - bus_driver_handle_get_connection_unix_process_id }, + bus_driver_handle_get_connection_unix_process_id, + METHOD_FLAG_ANY_PATH }, { "GetAdtAuditSessionData", DBUS_TYPE_STRING_AS_STRING, DBUS_TYPE_ARRAY_AS_STRING DBUS_TYPE_BYTE_AS_STRING, - bus_driver_handle_get_adt_audit_session_data }, + bus_driver_handle_get_adt_audit_session_data, + METHOD_FLAG_ANY_PATH }, { "GetConnectionSELinuxSecurityContext", DBUS_TYPE_STRING_AS_STRING, DBUS_TYPE_ARRAY_AS_STRING DBUS_TYPE_BYTE_AS_STRING, - bus_driver_handle_get_connection_selinux_security_context }, + bus_driver_handle_get_connection_selinux_security_context, + METHOD_FLAG_ANY_PATH }, { "ReloadConfig", "", "", - bus_driver_handle_reload_config }, + bus_driver_handle_reload_config, + METHOD_FLAG_ANY_PATH }, { "GetId", "", DBUS_TYPE_STRING_AS_STRING, - bus_driver_handle_get_id }, + bus_driver_handle_get_id, + METHOD_FLAG_ANY_PATH }, { "GetConnectionCredentials", "s", "a{sv}", - bus_driver_handle_get_connection_credentials }, + bus_driver_handle_get_connection_credentials, + METHOD_FLAG_ANY_PATH }, { NULL, NULL, NULL, NULL } }; @@ -2389,28 +2415,35 @@ static dbus_bool_t bus_driver_handle_introspect (DBusConnection *, BusTransaction *, DBusMessage *, DBusError *); static const MessageHandler introspectable_message_handlers[] = { - { "Introspect", "", DBUS_TYPE_STRING_AS_STRING, bus_driver_handle_introspect }, + { "Introspect", "", DBUS_TYPE_STRING_AS_STRING, bus_driver_handle_introspect, + METHOD_FLAG_ANY_PATH }, { NULL, NULL, NULL, NULL } }; static const MessageHandler monitoring_message_handlers[] = { - { "BecomeMonitor", "asu", "", bus_driver_handle_become_monitor }, + { "BecomeMonitor", "asu", "", bus_driver_handle_become_monitor, + METHOD_FLAG_NONE }, { NULL, NULL, NULL, NULL } }; #ifdef DBUS_ENABLE_VERBOSE_MODE static const MessageHandler verbose_message_handlers[] = { - { "EnableVerbose", "", "", bus_driver_handle_enable_verbose}, - { "DisableVerbose", "", "", bus_driver_handle_disable_verbose}, + { "EnableVerbose", "", "", bus_driver_handle_enable_verbose, + METHOD_FLAG_NONE }, + { "DisableVerbose", "", "", bus_driver_handle_disable_verbose, + METHOD_FLAG_NONE }, { NULL, NULL, NULL, NULL } }; #endif #ifdef DBUS_ENABLE_STATS static const MessageHandler stats_message_handlers[] = { - { "GetStats", "", "a{sv}", bus_stats_handle_get_stats }, - { "GetConnectionStats", "s", "a{sv}", bus_stats_handle_get_connection_stats }, - { "GetAllMatchRules", "", "a{sas}", bus_stats_handle_get_all_match_rules }, + { "GetStats", "", "a{sv}", bus_stats_handle_get_stats, + METHOD_FLAG_NONE }, + { "GetConnectionStats", "s", "a{sv}", bus_stats_handle_get_connection_stats, + METHOD_FLAG_NONE }, + { "GetAllMatchRules", "", "a{sas}", bus_stats_handle_get_all_match_rules, + METHOD_FLAG_NONE }, { NULL, NULL, NULL, NULL } }; #endif @@ -2613,38 +2646,6 @@ bus_driver_handle_introspect (DBusConnection *connection, return FALSE; } -/* - * Set @error and return FALSE if the message is not directed to the - * dbus-daemon by its canonical object path. This is hardening against - * system services with poorly-written security policy files, which - * might allow sending dangerously broad equivalence classes of messages - * such as "anything with this assumed-to-be-safe object path". - * - * dbus-daemon is unusual in that it normally ignores the object path - * of incoming messages; we need to keep that behaviour for the "read" - * read-only method calls like GetConnectionUnixUser for backwards - * compatibility, but it seems safer to be more restrictive for things - * intended to be root-only or privileged-developers-only. - * - * It is possible that there are other system services with the same - * quirk as dbus-daemon. - */ -dbus_bool_t -bus_driver_check_message_is_for_us (DBusMessage *message, - DBusError *error) -{ - if (!dbus_message_has_path (message, DBUS_PATH_DBUS)) - { - dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED, - "Method '%s' is only available at the canonical object path '%s'", - dbus_message_get_member (message), DBUS_PATH_DBUS); - - return FALSE; - } - - return TRUE; -} - dbus_bool_t bus_driver_handle_message (DBusConnection *connection, BusTransaction *transaction, @@ -2740,6 +2741,16 @@ bus_driver_handle_message (DBusConnection *connection, _dbus_verbose ("Found driver handler for %s\n", name); + if (!(canonical_path || (mh->flags & METHOD_FLAG_ANY_PATH))) + { + _DBUS_ASSERT_ERROR_IS_CLEAR (error); + dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED, + "Method '%s' is only available at the canonical object path '%s'", + dbus_message_get_member (message), DBUS_PATH_DBUS); + _DBUS_ASSERT_ERROR_IS_SET (error); + return FALSE; + } + if (!dbus_message_has_signature (message, mh->in_args)) { _DBUS_ASSERT_ERROR_IS_CLEAR (error); diff --git a/bus/driver.h b/bus/driver.h index 30513ac8..61bbf778 100644 --- a/bus/driver.h +++ b/bus/driver.h @@ -47,7 +47,5 @@ dbus_bool_t bus_driver_send_service_owner_changed (const char *service_name DBusError *error); dbus_bool_t bus_driver_generate_introspect_string (DBusString *xml, dbus_bool_t canonical_path); -dbus_bool_t bus_driver_check_message_is_for_us (DBusMessage *message, - DBusError *error); #endif /* BUS_DRIVER_H */ -- 2.11.0