From 18c494713a6a0395d29ecd45f0d3771fec45d2af Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 29 Jun 2023 16:06:39 +0100 Subject: [PATCH 1/3] userdb: Add proper error reporting when getting groups from a uid Previously, if dbus_connection_get_unix_user() succeeded but _dbus_unix_groups_from_uid() failed, then bus_connection_get_unix_groups() would incorrectly fail without setting the error indicator, resulting in "(null)" being logged, which is rather unhelpful. This also lets us distinguish between ENOMEM and other errors, such as the uid not existing in the system's user database. Fixes: 145fb99b (untitled refactoring commit, 2006-12-12) Helps: https://gitlab.freedesktop.org/dbus/dbus/-/issues/343 Signed-off-by: Simon McVittie --- bus/connection.c | 2 +- bus/policy.c | 2 +- dbus/dbus-sysdeps-util-unix.c | 6 ++++-- dbus/dbus-sysdeps-util-win.c | 15 ++++++++++++--- dbus/dbus-sysdeps.h | 3 ++- dbus/dbus-userdb-util.c | 15 ++++++++++----- dbus/dbus-userdb.h | 3 ++- test/internals/misc-internals.c | 6 +++--- 8 files changed, 35 insertions(+), 17 deletions(-) diff --git a/bus/connection.c b/bus/connection.c index c9c0bed7..0e1616d6 100644 --- a/bus/connection.c +++ b/bus/connection.c @@ -1079,7 +1079,7 @@ bus_connection_get_unix_groups (DBusConnection *connection, if (dbus_connection_get_unix_user (connection, &uid)) { - if (!_dbus_unix_groups_from_uid (uid, groups, n_groups)) + if (!_dbus_unix_groups_from_uid (uid, groups, n_groups, error)) { _dbus_verbose ("Did not get any groups for UID %lu\n", uid); diff --git a/bus/policy.c b/bus/policy.c index 74cb41bd..b6890c79 100644 --- a/bus/policy.c +++ b/bus/policy.c @@ -450,7 +450,7 @@ bus_policy_allow_unix_user (BusPolicy *policy, int n_group_ids; /* On OOM or error we always reject the user */ - if (!_dbus_unix_groups_from_uid (uid, &group_ids, &n_group_ids)) + if (!_dbus_unix_groups_from_uid (uid, &group_ids, &n_group_ids, NULL)) { _dbus_verbose ("Did not get any groups for UID %lu\n", uid); diff --git a/dbus/dbus-sysdeps-util-unix.c b/dbus/dbus-sysdeps-util-unix.c index 9fe7d55f..eb5654ea 100644 --- a/dbus/dbus-sysdeps-util-unix.c +++ b/dbus/dbus-sysdeps-util-unix.c @@ -992,14 +992,16 @@ _dbus_parse_unix_group_from_config (const DBusString *groupname, * @param uid the UID * @param group_ids return location for array of group IDs * @param n_group_ids return location for length of returned array + * @param error error location * @returns #TRUE if the UID existed and we got some credentials */ dbus_bool_t _dbus_unix_groups_from_uid (dbus_uid_t uid, dbus_gid_t **group_ids, - int *n_group_ids) + int *n_group_ids, + DBusError *error) { - return _dbus_groups_from_uid (uid, group_ids, n_group_ids); + return _dbus_groups_from_uid (uid, group_ids, n_group_ids, error); } /** diff --git a/dbus/dbus-sysdeps-util-win.c b/dbus/dbus-sysdeps-util-win.c index c572fcd0..5e4634ff 100644 --- a/dbus/dbus-sysdeps-util-win.c +++ b/dbus/dbus-sysdeps-util-win.c @@ -649,6 +649,13 @@ dbus_bool_t _dbus_windows_user_is_process_owner (const char *windows_sid) unix emulation functions - should be removed sometime in the future =====================================================================*/ +static void +set_unix_uid_unsupported (DBusError *error) +{ + dbus_set_error (error, DBUS_ERROR_NOT_SUPPORTED, + "UNIX user IDs not supported on Windows"); +} + /** * Checks to see if the UNIX user ID is at the console. * Should always fail on Windows (set the error to @@ -662,8 +669,7 @@ dbus_bool_t _dbus_unix_user_is_at_console (dbus_uid_t uid, DBusError *error) { - dbus_set_error (error, DBUS_ERROR_NOT_SUPPORTED, - "UNIX user IDs not supported on Windows\n"); + set_unix_uid_unsupported (error); return FALSE; } @@ -707,13 +713,16 @@ _dbus_parse_unix_user_from_config (const DBusString *username, * @param uid the UID * @param group_ids return location for array of group IDs * @param n_group_ids return location for length of returned array + * @param error error location * @returns #TRUE if the UID existed and we got some credentials */ dbus_bool_t _dbus_unix_groups_from_uid (dbus_uid_t uid, dbus_gid_t **group_ids, - int *n_group_ids) + int *n_group_ids, + DBusError *error) { + set_unix_uid_unsupported (error); return FALSE; } diff --git a/dbus/dbus-sysdeps.h b/dbus/dbus-sysdeps.h index e7e36ad6..33637337 100644 --- a/dbus/dbus-sysdeps.h +++ b/dbus/dbus-sysdeps.h @@ -298,7 +298,8 @@ dbus_bool_t _dbus_parse_unix_group_from_config (const DBusString *groupname, dbus_gid_t *gid_p); dbus_bool_t _dbus_unix_groups_from_uid (dbus_uid_t uid, dbus_gid_t **group_ids, - int *n_group_ids); + int *n_group_ids, + DBusError *error); dbus_bool_t _dbus_unix_user_is_at_console (dbus_uid_t uid, DBusError *error); dbus_bool_t _dbus_unix_user_is_process_owner (dbus_uid_t uid); diff --git a/dbus/dbus-userdb-util.c b/dbus/dbus-userdb-util.c index 1ca21eb7..0093ee49 100644 --- a/dbus/dbus-userdb-util.c +++ b/dbus/dbus-userdb-util.c @@ -373,31 +373,35 @@ _dbus_user_database_lookup_group (DBusUserDatabase *db, * @param uid the UID * @param group_ids return location for array of group IDs * @param n_group_ids return location for length of returned array + * @param error error to fill in on failure * @returns #TRUE if the UID existed and we got some credentials */ dbus_bool_t _dbus_groups_from_uid (dbus_uid_t uid, dbus_gid_t **group_ids, - int *n_group_ids) + int *n_group_ids, + DBusError *error) { DBusUserDatabase *db; const DBusUserInfo *info; *group_ids = NULL; *n_group_ids = 0; - /* FIXME: this can't distinguish ENOMEM from other errors */ if (!_dbus_user_database_lock_system ()) - return FALSE; + { + _DBUS_SET_OOM (error); + return FALSE; + } db = _dbus_user_database_get_system (); if (db == NULL) { + _DBUS_SET_OOM (error); _dbus_user_database_unlock_system (); return FALSE; } - if (!_dbus_user_database_get_uid (db, uid, - &info, NULL)) + if (!_dbus_user_database_get_uid (db, uid, &info, error)) { _dbus_user_database_unlock_system (); return FALSE; @@ -410,6 +414,7 @@ _dbus_groups_from_uid (dbus_uid_t uid, *group_ids = dbus_new (dbus_gid_t, info->n_group_ids); if (*group_ids == NULL) { + _DBUS_SET_OOM (error); _dbus_user_database_unlock_system (); return FALSE; } diff --git a/dbus/dbus-userdb.h b/dbus/dbus-userdb.h index fcb515c8..9026caa4 100644 --- a/dbus/dbus-userdb.h +++ b/dbus/dbus-userdb.h @@ -100,7 +100,8 @@ dbus_bool_t _dbus_get_user_id_and_primary_group (const DBusString *username, dbus_gid_t *gid_p); dbus_bool_t _dbus_groups_from_uid (dbus_uid_t uid, dbus_gid_t **group_ids, - int *n_group_ids); + int *n_group_ids, + DBusError *error); DBUS_PRIVATE_EXPORT dbus_bool_t _dbus_is_console_user (dbus_uid_t uid, DBusError *error); diff --git a/test/internals/misc-internals.c b/test/internals/misc-internals.c index a1777bb2..79b926f2 100644 --- a/test/internals/misc-internals.c +++ b/test/internals/misc-internals.c @@ -935,7 +935,7 @@ _dbus_userdb_test (const char *test_data_dir) dbus_uid_t uid; unsigned long *group_ids; int n_group_ids, i; - DBusError error; + DBusError error = DBUS_ERROR_INIT; if (!_dbus_username_from_current_process (&username)) _dbus_test_fatal ("didn't get username"); @@ -946,8 +946,8 @@ _dbus_userdb_test (const char *test_data_dir) if (!_dbus_get_user_id (username, &uid)) _dbus_test_fatal ("didn't get uid"); - if (!_dbus_groups_from_uid (uid, &group_ids, &n_group_ids)) - _dbus_test_fatal ("didn't get groups"); + if (!_dbus_groups_from_uid (uid, &group_ids, &n_group_ids, &error)) + _dbus_test_fatal ("didn't get groups: %s: %s", error.name, error.message); _dbus_test_diag (" Current user: %s homedir: %s gids:", _dbus_string_get_const_data (username), From f495f5ac0217c82bad5e4b57077b84743b20ca29 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 29 Jun 2023 16:54:46 +0100 Subject: [PATCH 2/3] test: Add a targeted test for _dbus_unix_groups_from_uid() Helps: https://gitlab.freedesktop.org/dbus/dbus/-/issues/343 Signed-off-by: Simon McVittie --- test/CMakeLists.txt | 1 + test/Makefile.am | 4 ++ test/internals/userdb.c | 143 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 148 insertions(+) create mode 100644 test/internals/userdb.c diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 1a21e711..e27e3285 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -201,6 +201,7 @@ if(DBUS_WITH_GLIB) add_test_executable(test-sysdeps internals/sysdeps.c ${TEST_LIBRARIES}) add_test_executable(test-syslog internals/syslog.c ${TEST_LIBRARIES}) add_test_executable(test-uid-permissions uid-permissions.c ${TEST_LIBRARIES}) + add_test_executable(test-userdb internals/userdb.c ${TEST_LIBRARIES}) add_helper_executable(manual-authz manual-authz.c ${TEST_LIBRARIES}) add_helper_executable(manual-test-thread-blocking thread-blocking.c ${TEST_LIBRARIES}) endif() diff --git a/test/Makefile.am b/test/Makefile.am index b6cd093a..878016e6 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -171,6 +171,9 @@ test_sysdeps_LDADD = libdbus-testutils.la $(GLIB_LIBS) test_syslog_SOURCES = internals/syslog.c test_syslog_LDADD = libdbus-testutils.la $(GLIB_LIBS) +test_userdb_SOURCES = internals/userdb.c +test_userdb_LDADD = libdbus-testutils.la $(GLIB_LIBS) + test_variant_SOURCES = internals/variant.c test_variant_LDADD = libdbus-testutils.la $(GLIB_LIBS) @@ -316,6 +319,7 @@ installable_tests += \ test-sysdeps \ test-syslog \ test-uid-permissions \ + test-userdb \ test-variant \ $(NULL) diff --git a/test/internals/userdb.c b/test/internals/userdb.c new file mode 100644 index 00000000..905791b3 --- /dev/null +++ b/test/internals/userdb.c @@ -0,0 +1,143 @@ +/* + * Copyright © 2023 Collabora Ltd. + * SPDX-License-Identifier: MIT + */ + +#include + +#include + +#include +#include "dbus/dbus-sysdeps.h" +#include "test-utils-glib.h" + +#ifdef DBUS_UNIX +#include +#include +#include +#include + +#include "dbus/dbus-sysdeps-unix.h" +#include "dbus/dbus-userdb.h" +#endif + +typedef struct +{ + int dummy; +} Fixture; + +static void +setup (Fixture *f G_GNUC_UNUSED, + gconstpointer context G_GNUC_UNUSED) +{ +} + +static void +test_groups_from_uid (Fixture *f, + gconstpointer context G_GNUC_UNUSED) +{ + DBusError error = DBUS_ERROR_INIT; + dbus_gid_t *gids = NULL; + int n_gids = -1; + dbus_bool_t ret; +#ifdef DBUS_UNIX + int i; +#endif + + /* We assume that uid 0 (root) is available on all Unix systems, + * so this should succeed */ + ret = _dbus_unix_groups_from_uid (0, &gids, &n_gids, &error); + +#ifdef DBUS_UNIX + test_assert_no_error (&error); + g_assert_true (ret); + g_assert_cmpint (n_gids, >=, 0); + + g_test_message ("Groups of uid 0:"); + + for (i = 0; i < n_gids; i++) + { + g_test_message ("[%d]: %ld", i, (long) gids[i]); + g_assert_cmpint (gids[i], >=, 0); + } +#else + g_assert_cmpstr (error.name, ==, DBUS_ERROR_NOT_SUPPORTED); + g_assert_false (ret); + g_test_message ("Getting Unix groups on Windows failed as expected: %s: %s", + error.name, error.message); + g_assert_null (gids); + g_assert_cmpint (n_gids, <=, 0); +#endif + + dbus_free (gids); + dbus_error_free (&error); + +#ifdef DBUS_UNIX + /* Assume that the current uid is something sensible */ + ret = _dbus_unix_groups_from_uid (geteuid (), &gids, &n_gids, &error); + test_assert_no_error (&error); + g_assert_true (ret); + g_assert_cmpint (n_gids, >=, 0); + + g_test_message ("Groups of uid %ld:", (long) geteuid ()); + + for (i = 0; i < n_gids; i++) + { + g_test_message ("[%d]: %ld", i, (long) gids[i]); + g_assert_cmpint (gids[i], >=, 0); + } + + g_test_message ("Total: %i groups", n_gids); + + dbus_free (gids); + dbus_error_free (&error); + + errno = 0; + + /* arbitrarily chosen, probably isn't a valid uid */ + if (getpwuid (31337) == NULL) + { + g_test_message ("uid 31337 doesn't exist: %s", + errno == 0 ? "(no errno)" : g_strerror (errno)); + ret = _dbus_unix_groups_from_uid (31337, &gids, &n_gids, &error); + g_assert_nonnull (error.name); + g_assert_nonnull (error.message); + g_assert_false (ret); + g_test_message ("Getting groups from non-uid failed as expected: %s: %s", + error.name, error.message); + /* The Unix implementation always clears gids/n_gids, + * even on failure, and even if they were uninitialized */ + g_assert_null (gids); + g_assert_cmpint (n_gids, ==, 0); + + dbus_free (gids); + dbus_error_free (&error); + } + else + { + g_test_skip ("against our expectations, uid 31337 exists on this system"); + } +#endif +} + +static void +teardown (Fixture *f G_GNUC_UNUSED, + gconstpointer context G_GNUC_UNUSED) +{ +} + +int +main (int argc, + char **argv) +{ + int ret; + + test_init (&argc, &argv); + + g_test_add ("/userdb/groups_from_uid", + Fixture, NULL, setup, test_groups_from_uid, teardown); + + ret = g_test_run (); + dbus_shutdown (); + return ret; +} From 9213da104d51ebf5e479dc187e1215935c70cdd1 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 29 Jun 2023 19:52:39 +0100 Subject: [PATCH 3/3] bus: When failing to reload client policy, continue iteration If we have a large number of connections to the bus, and we fail to reload the policy for one of them (perhaps because its uid no longer exists in the system user database), previously we would crash, which is obviously unintended. After the previous commit, we would stop iteration through the list of client connections, which doesn't seem great either: one bad connection shouldn't prevent us from reloading the rest of our state. Instead, let's distinguish between new connections (where we want failure to establish a security policy to be fatal), and pre-existing connections (where the current security policy is presumably good enough to keep using if we have nothing better). If we're unable to reload the policy for a pre-existing connection, log a warning and carry on iterating. Helps: https://gitlab.freedesktop.org/dbus/dbus/-/issues/343 Signed-off-by: Simon McVittie --- bus/bus.c | 35 +++++++++++++++++++++++++++++++++-- bus/bus.h | 1 + bus/connection.c | 2 ++ 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/bus/bus.c b/bus/bus.c index 9003860e..11de90dd 100644 --- a/bus/bus.c +++ b/bus/bus.c @@ -1412,11 +1412,42 @@ bus_context_get_containers (BusContext *context) BusClientPolicy* bus_context_create_client_policy (BusContext *context, DBusConnection *connection, + BusClientPolicy *previous, DBusError *error) { + BusClientPolicy *client; + DBusError local_error = DBUS_ERROR_INIT; + const char *conn; + const char *loginfo; + _DBUS_ASSERT_ERROR_IS_CLEAR (error); - return bus_policy_create_client_policy (context->policy, connection, - error); + + client = bus_policy_create_client_policy (context->policy, connection, + &local_error); + + /* On success, use new policy */ + if (client != NULL) + return client; + + /* On failure while setting up a new connection, fail */ + if (previous == NULL) + { + dbus_move_error (&local_error, error); + return NULL; + } + + /* On failure while reloading, keep the previous policy */ + conn = bus_connection_get_name (connection); + loginfo = bus_connection_get_loginfo (connection); + + if (conn == NULL) + conn = "(inactive)"; + + bus_context_log (context, DBUS_SYSTEM_LOG_WARNING, + "Unable to reload policy for connection \"%s\" (%s), " + "keeping current policy: %s", + conn, loginfo, local_error.message); + return bus_client_policy_ref (previous); } int diff --git a/bus/bus.h b/bus/bus.h index b4411180..bc8b42c3 100644 --- a/bus/bus.h +++ b/bus/bus.h @@ -115,6 +115,7 @@ BusContainers *bus_context_get_containers (BusContext BusClientPolicy* bus_context_create_client_policy (BusContext *context, DBusConnection *connection, + BusClientPolicy *previous, DBusError *error); int bus_context_get_activation_timeout (BusContext *context); int bus_context_get_auth_timeout (BusContext *context); diff --git a/bus/connection.c b/bus/connection.c index 0e1616d6..b912d89e 100644 --- a/bus/connection.c +++ b/bus/connection.c @@ -1584,6 +1584,7 @@ bus_connection_complete (DBusConnection *connection, d->policy = bus_context_create_client_policy (d->connections->context, connection, + NULL, error); /* we may have a NULL policy on OOM or error getting list of @@ -1669,6 +1670,7 @@ bus_connections_reload_policy (BusConnections *connections, policy = bus_context_create_client_policy (connections->context, connection, + d->policy, error); if (policy == NULL) {