From 3a1b1e9a4010e581e2e940e61d37c4f617eb5eff Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 5 Jun 2023 17:56:33 +0100 Subject: [PATCH 1/3] monitor test: Log the messages that we monitored This is helpful while debugging test failures. Helps: dbus/dbus#457 Signed-off-by: Simon McVittie (cherry picked from commit 8ee5d3e04420975107c27073b50f8758871a998b) --- test/monitor.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/monitor.c b/test/monitor.c index df5a7180..182110f8 100644 --- a/test/monitor.c +++ b/test/monitor.c @@ -196,6 +196,10 @@ _log_message (DBusMessage *m, not_null (dbus_message_get_signature (m))); g_test_message ("\terror name: %s", not_null (dbus_message_get_error_name (m))); + g_test_message ("\tserial number: %u", + dbus_message_get_serial (m)); + g_test_message ("\tin reply to: %u", + dbus_message_get_reply_serial (m)); if (strcmp ("s", dbus_message_get_signature (m)) == 0) { @@ -339,6 +343,9 @@ monitor_filter (DBusConnection *connection, { Fixture *f = user_data; + g_test_message ("Monitor received message:"); + log_message (message); + g_assert_cmpstr (dbus_message_get_interface (message), !=, "com.example.Tedious"); -- 2.41.0 From 37a4dc5835731a1f7a81f1b67c45b8dfb556dd1c Mon Sep 17 00:00:00 2001 From: hongjinghao Date: Mon, 5 Jun 2023 18:17:06 +0100 Subject: [PATCH 2/3] bus: Assign a serial number for messages from the driver Normally, it's enough to rely on a message being given a serial number by the DBusConnection just before it is actually sent. However, in the rare case where the policy blocks the driver from sending a message (due to a deny rule or the outgoing message quota being full), we need to get a valid serial number sooner, so that we can copy it into the DBUS_HEADER_FIELD_REPLY_SERIAL field (which is mandatory) in the error message sent to monitors. Otherwise, the dbus-daemon will crash with an assertion failure if at least one Monitoring client is attached, because zero is not a valid serial number to copy. This fixes a denial-of-service vulnerability: if a privileged user is monitoring the well-known system bus using a Monitoring client like dbus-monitor or `busctl monitor`, then an unprivileged user can cause denial-of-service by triggering this crash. A mitigation for this vulnerability is to avoid attaching Monitoring clients to the system bus when they are not needed. If there are no Monitoring clients, then the vulnerable code is not reached. Co-authored-by: Simon McVittie Resolves: dbus/dbus#457 (cherry picked from commit b159849e031000d1dbc1ab876b5fc78a3ce9b534) --- bus/connection.c | 15 +++++++++++++++ dbus/dbus-connection-internal.h | 2 ++ dbus/dbus-connection.c | 11 ++++++++++- 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/bus/connection.c b/bus/connection.c index b3583433..215f0230 100644 --- a/bus/connection.c +++ b/bus/connection.c @@ -2350,6 +2350,21 @@ bus_transaction_send_from_driver (BusTransaction *transaction, if (!dbus_message_set_sender (message, DBUS_SERVICE_DBUS)) return FALSE; + /* Make sure the message has a non-zero serial number, otherwise + * bus_transaction_capture_error_reply() will not be able to mock up + * a corresponding reply for it. Normally this would be delayed until + * the first time we actually send the message out from a + * connection, when the transaction is committed, but that's too late + * in this case. + */ + if (dbus_message_get_serial (message) == 0) + { + dbus_uint32_t next_serial; + + next_serial = _dbus_connection_get_next_client_serial (connection); + dbus_message_set_serial (message, next_serial); + } + if (bus_connection_is_active (connection)) { if (!dbus_message_set_destination (message, diff --git a/dbus/dbus-connection-internal.h b/dbus/dbus-connection-internal.h index 48357321..ba79b192 100644 --- a/dbus/dbus-connection-internal.h +++ b/dbus/dbus-connection-internal.h @@ -54,6 +54,8 @@ DBUS_PRIVATE_EXPORT DBusConnection * _dbus_connection_ref_unlocked (DBusConnection *connection); DBUS_PRIVATE_EXPORT void _dbus_connection_unref_unlocked (DBusConnection *connection); +DBUS_PRIVATE_EXPORT +dbus_uint32_t _dbus_connection_get_next_client_serial (DBusConnection *connection); void _dbus_connection_queue_received_message_link (DBusConnection *connection, DBusList *link); dbus_bool_t _dbus_connection_has_messages_to_send_unlocked (DBusConnection *connection); diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c index c525b6dc..09cef278 100644 --- a/dbus/dbus-connection.c +++ b/dbus/dbus-connection.c @@ -1456,7 +1456,16 @@ _dbus_connection_unref_unlocked (DBusConnection *connection) _dbus_connection_last_unref (connection); } -static dbus_uint32_t +/** + * Allocate and return the next non-zero serial number for outgoing messages. + * + * This method is only valid to call from single-threaded code, such as + * the dbus-daemon, or with the connection lock held. + * + * @param connection the connection + * @returns A suitable serial number for the next message to be sent on the connection. + */ +dbus_uint32_t _dbus_connection_get_next_client_serial (DBusConnection *connection) { dbus_uint32_t serial; -- 2.41.0 From 2c699f6ba9c162878c69d0728298c1ab7308db72 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 5 Jun 2023 18:51:22 +0100 Subject: [PATCH 3/3] monitor test: Reproduce dbus/dbus#457 The exact failure mode reported in dbus/dbus#457 is quite difficult to achieve in a reliable way in a unit test, because we'd have to send enough messages to a client to fill up its queue, then stop that client from draining its queue, while still triggering a message that gets a reply from the bus driver. However, we can trigger the same crash in a slightly different way by not allowing the client to receive a particular message. I chose NameAcquired. Signed-off-by: Simon McVittie (cherry picked from commit 986611ad0f7f67a3693e5672cd66bc608c00b228) --- .../valid-config-files/forbidding.conf.in | 3 + test/monitor.c | 77 ++++++++++++++++--- 2 files changed, 71 insertions(+), 9 deletions(-) diff --git a/test/data/valid-config-files/forbidding.conf.in b/test/data/valid-config-files/forbidding.conf.in index d145613c..58b3cc6a 100644 --- a/test/data/valid-config-files/forbidding.conf.in +++ b/test/data/valid-config-files/forbidding.conf.in @@ -24,5 +24,8 @@ + + + diff --git a/test/monitor.c b/test/monitor.c index 182110f8..42e0734d 100644 --- a/test/monitor.c +++ b/test/monitor.c @@ -155,6 +155,21 @@ static Config side_effects_config = { TRUE }; +static dbus_bool_t +config_forbids_name_acquired_signal (const Config *config) +{ + if (config == NULL) + return FALSE; + + if (config->config_file == NULL) + return FALSE; + + if (strcmp (config->config_file, forbidding_config.config_file) == 0) + return TRUE; + + return FALSE; +} + static inline const char * not_null2 (const char *x, const char *fallback) @@ -253,9 +268,6 @@ do { \ #define assert_name_acquired(m) \ do { \ - DBusError _e = DBUS_ERROR_INIT; \ - const char *_s; \ - \ g_assert_cmpstr (dbus_message_type_to_string (dbus_message_get_type (m)), \ ==, dbus_message_type_to_string (DBUS_MESSAGE_TYPE_SIGNAL)); \ g_assert_cmpstr (dbus_message_get_sender (m), ==, DBUS_SERVICE_DBUS); \ @@ -265,7 +277,14 @@ do { \ g_assert_cmpstr (dbus_message_get_signature (m), ==, "s"); \ g_assert_cmpint (dbus_message_get_serial (m), !=, 0); \ g_assert_cmpint (dbus_message_get_reply_serial (m), ==, 0); \ +} while (0) + +#define assert_unique_name_acquired(m) \ +do { \ + DBusError _e = DBUS_ERROR_INIT; \ + const char *_s; \ \ + assert_name_acquired (m); \ dbus_message_get_args (m, &_e, \ DBUS_TYPE_STRING, &_s, \ DBUS_TYPE_INVALID); \ @@ -333,6 +352,21 @@ do { \ g_assert_cmpint (dbus_message_get_reply_serial (m), !=, 0); \ } while (0) +/* forbidding.conf does not allow receiving NameAcquired, so if we are in + * that configuration, then dbus-daemon synthesizes an error reply to itself + * and sends that to monitors */ +#define expect_name_acquired_error(queue, in_reply_to) \ +do { \ + DBusMessage *message; \ + \ + message = g_queue_pop_head (queue); \ + assert_error_reply (message, DBUS_SERVICE_DBUS, DBUS_SERVICE_DBUS, \ + DBUS_ERROR_ACCESS_DENIED); \ + g_assert_cmpint (dbus_message_get_reply_serial (message), ==, \ + dbus_message_get_serial (in_reply_to)); \ + dbus_message_unref (message); \ +} while (0) + /* This is called after processing pending replies to our own method * calls, but before anything else. */ @@ -797,6 +831,11 @@ test_become_monitor (Fixture *f, test_assert_no_error (&f->e); g_assert_cmpint (ret, ==, DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER); + /* If the policy forbids receiving NameAcquired, then we'll never + * receive it, so behave as though we had */ + if (config_forbids_name_acquired_signal (f->config)) + got_unique = got_a = got_b = got_c = TRUE; + while (!got_unique || !got_a || !got_b || !got_c) { if (g_queue_is_empty (&f->monitored)) @@ -1448,6 +1487,7 @@ test_dbus_daemon (Fixture *f, { DBusMessage *m; int res; + size_t n_expected; if (f->address == NULL) return; @@ -1463,7 +1503,12 @@ test_dbus_daemon (Fixture *f, test_assert_no_error (&f->e); g_assert_cmpint (res, ==, DBUS_RELEASE_NAME_REPLY_RELEASED); - while (g_queue_get_length (&f->monitored) < 8) + n_expected = 8; + + if (config_forbids_name_acquired_signal (context)) + n_expected += 1; + + while (g_queue_get_length (&f->monitored) < n_expected) test_main_context_iterate (f->ctx, TRUE); m = g_queue_pop_head (&f->monitored); @@ -1476,10 +1521,12 @@ test_dbus_daemon (Fixture *f, "NameOwnerChanged", "sss", NULL); dbus_message_unref (m); - /* FIXME: should we get this? */ m = g_queue_pop_head (&f->monitored); - assert_signal (m, DBUS_SERVICE_DBUS, DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, - "NameAcquired", "s", f->sender_name); + assert_name_acquired (m); + + if (config_forbids_name_acquired_signal (f->config)) + expect_name_acquired_error (&f->monitored, m); + dbus_message_unref (m); m = g_queue_pop_head (&f->monitored); @@ -1701,8 +1748,14 @@ static void expect_new_connection (Fixture *f) { DBusMessage *m; + size_t n_expected; - while (g_queue_get_length (&f->monitored) < 4) + n_expected = 4; + + if (config_forbids_name_acquired_signal (f->config)) + n_expected += 1; + + while (g_queue_get_length (&f->monitored) < n_expected) test_main_context_iterate (f->ctx, TRUE); m = g_queue_pop_head (&f->monitored); @@ -1719,7 +1772,11 @@ expect_new_connection (Fixture *f) dbus_message_unref (m); m = g_queue_pop_head (&f->monitored); - assert_name_acquired (m); + assert_unique_name_acquired (m); + + if (config_forbids_name_acquired_signal (f->config)) + expect_name_acquired_error (&f->monitored, m); + dbus_message_unref (m); } @@ -2044,6 +2101,8 @@ main (int argc, setup, test_method_call, teardown); g_test_add ("/monitor/forbidden-method", Fixture, &forbidding_config, setup, test_forbidden_method_call, teardown); + g_test_add ("/monitor/forbidden-reply", Fixture, &forbidding_config, + setup, test_dbus_daemon, teardown); g_test_add ("/monitor/dbus-daemon", Fixture, NULL, setup, test_dbus_daemon, teardown); g_test_add ("/monitor/selective", Fixture, &selective_config, -- 2.41.0