diff options
author | Simon McVittie <smcv@collabora.com> | 2022-10-11 15:58:07 +0000 |
---|---|---|
committer | Simon McVittie <smcv@collabora.com> | 2022-10-11 15:58:07 +0000 |
commit | 79f7fcab69ec13fdb66deb9ca4d58f9565de82db (patch) | |
tree | 580e1b03d3a9877e807d05851951f4c72e8bf4c9 | |
parent | 2a546c1acb89cfe2cc16c2f20a1feda0002e9088 (diff) | |
parent | dbb1b8f5e66ec1bb77b5fba5d7aed7fee059d235 (diff) | |
download | dbus-79f7fcab69ec13fdb66deb9ca4d58f9565de82db.tar.gz |
Merge branch '1.14-backports' into 'dbus-1.14'
[1.14.x] Backport fixes from master
See merge request dbus/dbus!362
-rw-r--r-- | NEWS | 14 | ||||
-rw-r--r-- | bus/config-parser.c | 4 | ||||
-rw-r--r-- | dbus/dbus-connection-internal.h | 3 | ||||
-rw-r--r-- | dbus/dbus-connection.c | 47 | ||||
-rw-r--r-- | dbus/dbus-message.c | 3 | ||||
-rw-r--r-- | test/loopback.c | 87 | ||||
-rw-r--r-- | test/message.c | 9 | ||||
-rw-r--r-- | test/monitor.c | 4 | ||||
-rw-r--r-- | tools/dbus-monitor.c | 3 |
9 files changed, 156 insertions, 18 deletions
@@ -1,7 +1,19 @@ dbus 1.14.6 (UNRELEASED) ======================== -... +Fixes: + +• When connected to a dbus-broker, stop dbus-monitor from incorrectly + replying to Peer method calls that were sent to the dbus-broker with + a NULL destination (dbus#301, Kai A. Hiller) + +• Fix out-of-bounds varargs read in the dbus-daemon's config-parser. + This is not attacker-triggerable and appears to be harmless in practice, + but is technically undefined behaviour and is detected as such by + AddressSanitizer. (dbus!357, Evgeny Vereshchagin) + +• If dbus_message_demarshal() runs out of memory while validating a message, + report it as NoMemory rather than InvalidArgs (dbus#420, Simon McVittie) dbus 1.14.4 (2022-10-05) ======================== diff --git a/bus/config-parser.c b/bus/config-parser.c index f9b70477..9f2e3c79 100644 --- a/bus/config-parser.c +++ b/bus/config-parser.c @@ -642,10 +642,11 @@ locate_attributes (BusConfigParser *parser, va_start (args, first_attribute_retloc); name = va_arg (args, const char*); - retloc = va_arg (args, const char**); + retloc = NULL; while (name != NULL) { + retloc = va_arg (args, const char**); _dbus_assert (retloc != NULL); _dbus_assert (n_attrs < MAX_ATTRS); @@ -655,7 +656,6 @@ locate_attributes (BusConfigParser *parser, *retloc = NULL; name = va_arg (args, const char*); - retloc = va_arg (args, const char**); } va_end (args); diff --git a/dbus/dbus-connection-internal.h b/dbus/dbus-connection-internal.h index de4e55b1..d6947d18 100644 --- a/dbus/dbus-connection-internal.h +++ b/dbus/dbus-connection-internal.h @@ -120,6 +120,9 @@ dbus_bool_t _dbus_connection_get_linux_security_label (DBusConnectio char **label_p); DBUS_PRIVATE_EXPORT DBusCredentials *_dbus_connection_get_credentials (DBusConnection *connection); +DBUS_PRIVATE_EXPORT +void _dbus_connection_set_builtin_filters_enabled (DBusConnection *connection, + dbus_bool_t value); /* if DBUS_ENABLE_STATS */ DBUS_PRIVATE_EXPORT diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c index 159dbe1b..a61e56a6 100644 --- a/dbus/dbus-connection.c +++ b/dbus/dbus-connection.c @@ -316,6 +316,8 @@ struct DBusConnection unsigned int exit_on_disconnect : 1; /**< If #TRUE, exit after handling disconnect signal */ + unsigned int builtin_filters_enabled : 1; /**< If #TRUE, handle org.freedesktop.DBus.Peer messages automatically, whether they have a bus name or not */ + unsigned int route_peer_messages : 1; /**< If #TRUE, if org.freedesktop.DBus.Peer messages have a bus name, don't handle them automatically */ unsigned int disconnected_message_arrived : 1; /**< We popped or are dispatching the disconnected message. @@ -1343,6 +1345,7 @@ _dbus_connection_new_for_transport (DBusTransport *transport) connection->objects = objects; connection->exit_on_disconnect = FALSE; connection->shareable = FALSE; + connection->builtin_filters_enabled = TRUE; connection->route_peer_messages = FALSE; connection->disconnected_message_arrived = FALSE; connection->disconnected_message_processed = FALSE; @@ -4657,10 +4660,14 @@ dbus_connection_dispatch (DBusConnection *connection) goto out; } - result = _dbus_connection_run_builtin_filters_unlocked_no_update (connection, message); - if (result != DBUS_HANDLER_RESULT_NOT_YET_HANDLED) - goto out; - + /* If skipping builtin filters, we are probably a monitor. */ + if (connection->builtin_filters_enabled) + { + result = _dbus_connection_run_builtin_filters_unlocked_no_update (connection, message); + if (result != DBUS_HANDLER_RESULT_NOT_YET_HANDLED) + goto out; + } + if (!_dbus_list_copy (&connection->filter_list, &filter_list_copy)) { _dbus_connection_release_dispatch (connection); @@ -5533,6 +5540,38 @@ dbus_connection_set_allow_anonymous (DBusConnection *connection, } /** + * Enables the builtin filtering of messages. + * + * Currently the only filtering implemented by libdbus and mandated by the spec + * is that of peer messages. + * + * If #TRUE, #DBusConnection automatically handles all messages to the + * org.freedesktop.DBus.Peer interface. For monitors this can break the + * specification if the response is sending a message. + * + * If #FALSE, the result is similar to calling + * dbus_connection_set_route_peer_messages() with argument TRUE, but + * messages with a NULL destination are also dispatched to the + * application instead of being passed to the built-in filters. + * + * If a normal application disables this flag, it can break things badly. So + * only unset this if you are a monitor. + * + * @param connection the connection + * @param value #TRUE to pass through org.freedesktop.DBus.Peer messages + */ +void +_dbus_connection_set_builtin_filters_enabled (DBusConnection *connection, + dbus_bool_t value) +{ + _dbus_assert (connection != NULL); + + CONNECTION_LOCK (connection); + connection->builtin_filters_enabled = value; + CONNECTION_UNLOCK (connection); +} + +/** * * Normally #DBusConnection automatically handles all messages to the * org.freedesktop.DBus.Peer interface. However, the message bus wants diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c index 45b1adfd..9bc13cf9 100644 --- a/dbus/dbus-message.c +++ b/dbus/dbus-message.c @@ -5185,6 +5185,9 @@ dbus_message_demarshal (const char *str, return msg; fail_corrupt: + if (loader->corruption_reason == DBUS_VALIDITY_UNKNOWN_OOM_ERROR) + goto fail_oom; + dbus_set_error (error, DBUS_ERROR_INVALID_ARGS, "Message is corrupted (%s)", _dbus_validity_to_error_message (loader->corruption_reason)); _dbus_message_loader_unref (loader); diff --git a/test/loopback.c b/test/loopback.c index f89f5a95..bca0d3c2 100644 --- a/test/loopback.c +++ b/test/loopback.c @@ -61,6 +61,37 @@ assert_no_error (const DBusError *e) g_error ("expected success but got error: %s: %s", e->name, e->message); } +/* these are macros so they get the right line number */ + +#define assert_method_reply(m, sender, destination, signature) \ +do { \ + g_assert_cmpstr (dbus_message_type_to_string (dbus_message_get_type (m)), \ + ==, dbus_message_type_to_string (DBUS_MESSAGE_TYPE_METHOD_RETURN)); \ + g_assert_cmpstr (dbus_message_get_sender (m), ==, sender); \ + g_assert_cmpstr (dbus_message_get_destination (m), ==, destination); \ + g_assert_cmpstr (dbus_message_get_path (m), ==, NULL); \ + g_assert_cmpstr (dbus_message_get_interface (m), ==, NULL); \ + g_assert_cmpstr (dbus_message_get_member (m), ==, NULL); \ + g_assert_cmpstr (dbus_message_get_signature (m), ==, signature); \ + g_assert_cmpint (dbus_message_get_serial (m), !=, 0); \ + g_assert_cmpint (dbus_message_get_reply_serial (m), !=, 0); \ +} while (0) + +#define assert_error_reply(m, sender, destination, error_name) \ +do { \ + g_assert_cmpstr (dbus_message_type_to_string (dbus_message_get_type (m)), \ + ==, dbus_message_type_to_string (DBUS_MESSAGE_TYPE_ERROR)); \ + g_assert_cmpstr (dbus_message_get_sender (m), ==, sender); \ + g_assert_cmpstr (dbus_message_get_destination (m), ==, destination); \ + g_assert_cmpstr (dbus_message_get_error_name (m), ==, error_name); \ + g_assert_cmpstr (dbus_message_get_path (m), ==, NULL); \ + g_assert_cmpstr (dbus_message_get_interface (m), ==, NULL); \ + g_assert_cmpstr (dbus_message_get_member (m), ==, NULL); \ + 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) + static DBusHandlerResult server_message_cb (DBusConnection *server_conn, DBusMessage *message, @@ -416,6 +447,59 @@ test_message (Fixture *f, } static void +test_builtin_filters (Fixture *f, + gconstpointer addr) +{ + dbus_bool_t have_mem; + dbus_uint32_t serial; + DBusMessage *ping; + DBusMessage *m; + + if (f->skip) + return; + + test_connect (f, addr); + + ping = dbus_message_new_method_call (NULL, "/foo", DBUS_INTERFACE_PEER, + "Ping"); + + _dbus_connection_set_builtin_filters_enabled (f->client_conn, TRUE); + + have_mem = dbus_connection_send (f->server_conn, ping, &serial); + g_assert (have_mem); + g_assert (serial != 0); + + while (g_queue_get_length (&f->server_messages) < 1) + test_main_context_iterate (f->ctx, TRUE); + + m = g_queue_pop_head (&f->server_messages); + assert_method_reply (m, NULL, NULL, ""); + dbus_message_unref (m); + + m = g_queue_pop_head (&f->server_messages); + g_assert (m == NULL); + + _dbus_connection_set_builtin_filters_enabled (f->client_conn, FALSE); + + have_mem = dbus_connection_send (f->server_conn, ping, &serial); + g_assert (have_mem); + g_assert (serial != 0); + + while (g_queue_get_length (&f->server_messages) < 1) + test_main_context_iterate (f->ctx, TRUE); + + m = g_queue_pop_head (&f->server_messages); + assert_error_reply (m, NULL, NULL, + "org.freedesktop.DBus.Error.UnknownMethod"); + dbus_message_unref (m); + + m = g_queue_pop_head (&f->server_messages); + g_assert (m == NULL); + + dbus_message_unref (ping); +} + +static void teardown (Fixture *f, gconstpointer addr G_GNUC_UNUSED) { @@ -523,6 +607,9 @@ main (int argc, test_bad_guid, teardown); #endif + g_test_add ("/builtin-filters", Fixture, "tcp:host=127.0.0.1", setup, + test_builtin_filters, teardown); + ret = g_test_run (); dbus_shutdown (); return ret; diff --git a/test/message.c b/test/message.c index 60ef113d..d2c9db2b 100644 --- a/test/message.c +++ b/test/message.c @@ -291,15 +291,6 @@ test_valid_message_blobs (void *message_name, goto out; } - /* TODO: Validity checking sometimes returns InvalidArgs for OOM */ - if (dbus_error_has_name (&e, DBUS_ERROR_INVALID_ARGS) && - !have_memory && - strstr (e.message, "Out of memory") != NULL) - { - g_test_message ("Out of memory (not a problem)"); - goto out; - } - g_test_message ("Parsing %s reported unexpected error %s: %s", path, e.name, e.message); g_test_fail (); diff --git a/test/monitor.c b/test/monitor.c index eb11eb81..d8005c87 100644 --- a/test/monitor.c +++ b/test/monitor.c @@ -28,6 +28,8 @@ #include <string.h> +#include "dbus/dbus-connection-internal.h" + #include "test-utils-glib.h" typedef struct { @@ -505,7 +507,7 @@ become_monitor (Fixture *f, int i; dbus_uint32_t zero = 0; - dbus_connection_set_route_peer_messages (f->monitor, TRUE); + _dbus_connection_set_builtin_filters_enabled (f->monitor, FALSE); if (config == NULL) config = f->config; diff --git a/tools/dbus-monitor.c b/tools/dbus-monitor.c index fcc923ee..bd111488 100644 --- a/tools/dbus-monitor.c +++ b/tools/dbus-monitor.c @@ -21,6 +21,7 @@ #include <config.h> +#include "dbus/dbus-connection-internal.h" #include "dbus/dbus-internals.h" #include <stdio.h> @@ -494,7 +495,7 @@ main (int argc, char *argv[]) /* Receive o.fd.Peer messages as normal messages, rather than having * libdbus handle them internally, which is the wrong thing for * a monitor */ - dbus_connection_set_route_peer_messages (connection, TRUE); + _dbus_connection_set_builtin_filters_enabled (connection, FALSE); if (!dbus_connection_add_filter (connection, filter_func, _DBUS_INT_TO_POINTER (binary_mode), NULL)) |