Merge branch '1.14-backports' into 'dbus-1.14'

[1.14.x] Backport fixes from master

See merge request dbus/dbus!362

NEWS

@@ -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)
 ========================

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);

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

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);
@@ -5532,6 +5539,38 @@ dbus_connection_set_allow_anonymous (DBusConnection             *connection,
   CONNECTION_UNLOCK (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

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);

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,
@@ -415,6 +446,59 @@ test_message (Fixture *f,
   dbus_clear_message (&outgoing);
 }
 
+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;

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 ();

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;

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))