Skip to content
Snippets Groups Projects
Commit 8e78f993 authored by Simon McVittie's avatar Simon McVittie Committed by Dylan Aïssi
Browse files

Import Debian changes 2.66.8-1+deb11u3

parent cb33f717
No related branches found
Tags debian/2.66.8-1+deb11u3
5 merge requests!37Merge changes from apertis/v2023-security into apertis/v2023,!34Update from debian/bullseye for apertis/v2022-updates,!32Update from debian/bullseye-security for apertis/v2022-security,!28Update from debian/bullseye-security for apertis/v2022-security,!27Update from debian/bullseye-security for apertis/v2023-security
Pipeline #710668 passed
Showing
with 3270 additions and 0 deletions
glib2.0 (2.66.8-1+deb11u3) bullseye-security; urgency=high
* d/p/CVE-2024-34397/gdbusconnection-Allow-name-owners-to-have-the-syntax-of-a.patch:
Relax name owner checks to avoid a regression in ibus.
Fixing CVE-2024-34397 caused a regression in ibus affecting text entry
with non-trivial input methods.
(Closes: #1070730, #1070736, #1070743, #1070745, #1070749, #1070752)
-- Simon McVittie <smcv@debian.org> Wed, 08 May 2024 16:25:40 +0100
glib2.0 (2.66.8-1+deb11u2) bullseye-security; urgency=high
* d/patches: Backport GDBus fixes from 2.80.1
- If local users send signals on the D-Bus system bus that spoof a
trusted sender, do not deliver them to signal subscriptions for the
trusted sender's well-known bus name (CVE-2024-34397)
- Fix a use-after-free when subscribing to signals with an arg0
match rule, originally from 2.79.0 and necessary to make the test
for CVE-2024-34397 pass reliably
- Add a local backport of g_set_str(), required by the above
-- Simon McVittie <smcv@debian.org> Mon, 06 May 2024 13:50:11 +0100
glib2.0 (2.66.8-1+deb11u1) bullseye; urgency=medium
* d/patches: Update to upstream commit 2.66.8-1-g284b7eb7f
......
From: Simon McVittie <smcv@debian.org>
Date: Wed, 8 May 2024 10:09:40 +0100
Subject: gdbusconnection: Allow name owners to have the syntax of a
well-known name
In a D-Bus-Specification-compliant message bus, the owner of a well-known
name is a unique name. However, ibus has its own small implementation
of a message bus (src/ibusbus.c) in which org.freedesktop.IBus is
special-cased to also have itself as its owner (like org.freedesktop.DBus
on a standard message bus), and connects to that bus with the
G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION flag. The ability to do
this regressed when CVE-2024-34397 was fixed.
Relax the checks to allow the owner of a well-known name to be any valid
D-Bus name, even if it is not syntactically a unique name.
Bug: https://gitlab.gnome.org/GNOME/glib/-/issues/3353
Bug-Debian: https://bugs.debian.org/1070730
Bug-Debian: https://bugs.debian.org/1070736
Bug-Debian: https://bugs.debian.org/1070743
Bug-Debian: https://bugs.debian.org/1070745
Signed-off-by: Simon McVittie <smcv@debian.org>
Forwarded: https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4053
---
gio/gdbusconnection.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index 311e045..42ef6fd 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -2400,7 +2400,7 @@ name_watcher_deliver_name_owner_changed_unlocked (SignalData *name_watcher,
/* Our caller already checked this */
g_assert (g_strcmp0 (name_watcher->arg0, name) == 0);
- if (G_LIKELY (new_owner[0] == '\0' || g_dbus_is_unique_name (new_owner)))
+ if (G_LIKELY (new_owner[0] == '\0' || g_dbus_is_name (new_owner)))
name_watcher_set_name_owner_unlocked (name_watcher, new_owner);
else
g_warning ("Received NameOwnerChanged signal with invalid owner \"%s\" for \"%s\"",
@@ -2452,7 +2452,7 @@ name_watcher_deliver_get_name_owner_reply_unlocked (SignalData *name_watcher,
g_variant_get (body, "(&s)", &new_owner);
- if (G_LIKELY (g_dbus_is_unique_name (new_owner)))
+ if (G_LIKELY (g_dbus_is_name (new_owner)))
name_watcher_set_name_owner_unlocked (name_watcher, new_owner);
else
g_warning ("Received GetNameOwner reply with invalid owner \"%s\" for \"%s\"",
From: Simon McVittie <smcv@collabora.com>
Date: Thu, 14 Mar 2024 20:42:41 +0000
Subject: gdbusconnection: Don't deliver signals if the sender doesn't match
Otherwise a malicious connection on a shared bus, especially the system
bus, could trick GDBus clients into processing signals sent by the
malicious connection as though they had come from the real owner of a
well-known service name.
Resolves: https://gitlab.gnome.org/GNOME/glib/-/issues/3268
Signed-off-by: Simon McVittie <smcv@collabora.com>
Origin: upstream, https://gitlab.gnome.org/GNOME/glib/-/issues/3268
---
gio/gdbusconnection.c | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index 454b17a..311e045 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -4285,6 +4285,46 @@ schedule_callbacks (GDBusConnection *connection,
if (signal_data->object_path != NULL && g_strcmp0 (signal_data->object_path, path) != 0)
continue;
+ if (signal_data->shared_name_watcher != NULL)
+ {
+ /* We want signals from a specified well-known name, which means
+ * the signal's sender needs to be the unique name that currently
+ * owns that well-known name, and we will have found this
+ * SignalData in
+ * connection->map_sender_unique_name_to_signal_data_array[""]. */
+ const WatchedName *watched_name;
+ const char *current_owner;
+
+ g_assert (signal_data->sender != NULL);
+ /* Invariant: We never need to watch for the owner of a unique
+ * name, or for the owner of DBUS_SERVICE_DBUS, either of which
+ * is always its own owner */
+ g_assert (!g_dbus_is_unique_name (signal_data->sender));
+ g_assert (g_strcmp0 (signal_data->sender, DBUS_SERVICE_DBUS) != 0);
+
+ watched_name = signal_data->shared_name_watcher->watched_name;
+ g_assert (watched_name != NULL);
+ current_owner = watched_name->owner;
+
+ /* Skip the signal if the actual sender is not known to own
+ * the required name */
+ if (current_owner == NULL || g_strcmp0 (current_owner, sender) != 0)
+ continue;
+ }
+ else if (signal_data->sender != NULL)
+ {
+ /* We want signals from a unique name or o.fd.DBus... */
+ g_assert (g_dbus_is_unique_name (signal_data->sender)
+ || g_str_equal (signal_data->sender, DBUS_SERVICE_DBUS));
+
+ /* ... which means we must have found this SignalData in
+ * connection->map_sender_unique_name_to_signal_data_array[signal_data->sender],
+ * therefore we would only have found it if the signal's
+ * actual sender matches the required signal_data->sender */
+ g_assert (g_strcmp0 (signal_data->sender, sender) == 0);
+ }
+ /* else the sender is unspecified and we will accept anything */
+
if (signal_data->arg0 != NULL)
{
if (arg0 == NULL)
From: Simon McVittie <smcv@collabora.com>
Date: Tue, 23 Apr 2024 20:31:57 +0100
Subject: gdbusconnection: Factor out add_signal_data()
No functional changes.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Origin: upstream, https://gitlab.gnome.org/GNOME/glib/-/issues/3268
---
gio/gdbusconnection.c | 64 +++++++++++++++++++++++++++++----------------------
1 file changed, 37 insertions(+), 27 deletions(-)
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index da08341..830230d 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -3451,6 +3451,42 @@ is_signal_data_for_name_lost_or_acquired (SignalData *signal_data)
/* ---------------------------------------------------------------------------------------------------- */
+/* called in any thread, connection lock is held */
+static void
+add_signal_data (GDBusConnection *connection,
+ SignalData *signal_data)
+{
+ GPtrArray *signal_data_array;
+
+ g_hash_table_insert (connection->map_rule_to_signal_data,
+ signal_data->rule,
+ signal_data);
+
+ /* Add the match rule to the bus...
+ *
+ * Avoid adding match rules for NameLost and NameAcquired messages - the bus will
+ * always send such messages to us.
+ */
+ if (connection->flags & G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION)
+ {
+ if (!is_signal_data_for_name_lost_or_acquired (signal_data))
+ add_match_rule (connection, signal_data->rule);
+ }
+
+ signal_data_array = g_hash_table_lookup (connection->map_sender_unique_name_to_signal_data_array,
+ signal_data->sender_unique_name);
+ if (signal_data_array == NULL)
+ {
+ signal_data_array = g_ptr_array_new ();
+ g_hash_table_insert (connection->map_sender_unique_name_to_signal_data_array,
+ g_strdup (signal_data->sender_unique_name),
+ signal_data_array);
+ }
+ g_ptr_array_add (signal_data_array, signal_data);
+}
+
+/* ---------------------------------------------------------------------------------------------------- */
+
/**
* g_dbus_connection_signal_subscribe:
* @connection: a #GDBusConnection
@@ -3540,7 +3576,6 @@ g_dbus_connection_signal_subscribe (GDBusConnection *connection,
gchar *rule;
SignalData *signal_data;
SignalSubscriber *subscriber;
- GPtrArray *signal_data_array;
const gchar *sender_unique_name;
/* Right now we abort if AddMatch() fails since it can only fail with the bus being in
@@ -3606,32 +3641,7 @@ g_dbus_connection_signal_subscribe (GDBusConnection *connection,
g_strdup (arg0),
flags);
g_ptr_array_add (signal_data->subscribers, subscriber);
-
- g_hash_table_insert (connection->map_rule_to_signal_data,
- signal_data->rule,
- signal_data);
-
- /* Add the match rule to the bus...
- *
- * Avoid adding match rules for NameLost and NameAcquired messages - the bus will
- * always send such messages to us.
- */
- if (connection->flags & G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION)
- {
- if (!is_signal_data_for_name_lost_or_acquired (signal_data))
- add_match_rule (connection, signal_data->rule);
- }
-
- signal_data_array = g_hash_table_lookup (connection->map_sender_unique_name_to_signal_data_array,
- signal_data->sender_unique_name);
- if (signal_data_array == NULL)
- {
- signal_data_array = g_ptr_array_new ();
- g_hash_table_insert (connection->map_sender_unique_name_to_signal_data_array,
- g_strdup (signal_data->sender_unique_name),
- signal_data_array);
- }
- g_ptr_array_add (signal_data_array, signal_data);
+ add_signal_data (connection, signal_data);
out:
g_hash_table_insert (connection->map_id_to_signal_data,
From: Simon McVittie <smcv@collabora.com>
Date: Thu, 14 Mar 2024 19:51:59 +0000
Subject: gdbusconnection: Factor out remove_signal_data_if_unused
No functional change, just removing some nesting. The check for whether
signal_data->subscribers is empty changes from a conditional that tests
whether it is into an early-return if it isn't.
A subsequent commit will add additional conditions that make us consider
a SignalData to be still in use and therefore not eligible to be removed.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Origin: upstream, https://gitlab.gnome.org/GNOME/glib/-/issues/3268
---
gio/gdbusconnection.c | 83 +++++++++++++++++++++++++++++----------------------
1 file changed, 48 insertions(+), 35 deletions(-)
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index 830230d..821132e 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -3655,6 +3655,52 @@ g_dbus_connection_signal_subscribe (GDBusConnection *connection,
/* ---------------------------------------------------------------------------------------------------- */
+/*
+ * Called in any thread.
+ * Must hold the connection lock when calling this, unless
+ * connection->finalizing is TRUE.
+ * May free signal_data, so do not dereference it after this.
+ */
+static void
+remove_signal_data_if_unused (GDBusConnection *connection,
+ SignalData *signal_data)
+{
+ GPtrArray *signal_data_array;
+
+ if (signal_data->subscribers->len != 0)
+ return;
+
+ g_warn_if_fail (g_hash_table_remove (connection->map_rule_to_signal_data, signal_data->rule));
+
+ signal_data_array = g_hash_table_lookup (connection->map_sender_unique_name_to_signal_data_array,
+ signal_data->sender_unique_name);
+ g_warn_if_fail (signal_data_array != NULL);
+ g_warn_if_fail (g_ptr_array_remove (signal_data_array, signal_data));
+
+ if (signal_data_array->len == 0)
+ {
+ g_warn_if_fail (g_hash_table_remove (connection->map_sender_unique_name_to_signal_data_array,
+ signal_data->sender_unique_name));
+ }
+
+ /* remove the match rule from the bus unless NameLost or NameAcquired (see subscribe()) */
+ if ((connection->flags & G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION) &&
+ !is_signal_data_for_name_lost_or_acquired (signal_data) &&
+ !g_dbus_connection_is_closed (connection) &&
+ !connection->finalizing)
+ {
+ /* The check for g_dbus_connection_is_closed() means that
+ * sending the RemoveMatch message can't fail with
+ * G_IO_ERROR_CLOSED, because we're holding the lock,
+ * so on_worker_closed() can't happen between the check we just
+ * did, and releasing the lock later.
+ */
+ remove_match_rule (connection, signal_data->rule);
+ }
+
+ signal_data_free (signal_data);
+}
+
/* called in any thread */
/* must hold lock when calling this (except if connection->finalizing is TRUE)
* returns the number of removed subscribers */
@@ -3663,7 +3709,6 @@ unsubscribe_id_internal (GDBusConnection *connection,
guint subscription_id)
{
SignalData *signal_data;
- GPtrArray *signal_data_array;
guint n;
guint n_removed = 0;
@@ -3690,40 +3735,8 @@ unsubscribe_id_internal (GDBusConnection *connection,
GUINT_TO_POINTER (subscription_id)));
n_removed++;
g_ptr_array_remove_index_fast (signal_data->subscribers, n);
-
- if (signal_data->subscribers->len == 0)
- {
- g_warn_if_fail (g_hash_table_remove (connection->map_rule_to_signal_data, signal_data->rule));
-
- signal_data_array = g_hash_table_lookup (connection->map_sender_unique_name_to_signal_data_array,
- signal_data->sender_unique_name);
- g_warn_if_fail (signal_data_array != NULL);
- g_warn_if_fail (g_ptr_array_remove (signal_data_array, signal_data));
-
- if (signal_data_array->len == 0)
- {
- g_warn_if_fail (g_hash_table_remove (connection->map_sender_unique_name_to_signal_data_array,
- signal_data->sender_unique_name));
- }
-
- /* remove the match rule from the bus unless NameLost or NameAcquired (see subscribe()) */
- if ((connection->flags & G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION) &&
- !is_signal_data_for_name_lost_or_acquired (signal_data) &&
- !g_dbus_connection_is_closed (connection) &&
- !connection->finalizing)
- {
- /* The check for g_dbus_connection_is_closed() means that
- * sending the RemoveMatch message can't fail with
- * G_IO_ERROR_CLOSED, because we're holding the lock,
- * so on_worker_closed() can't happen between the check we just
- * did, and releasing the lock later.
- */
- remove_match_rule (connection, signal_data->rule);
- }
-
- signal_data_free (signal_data);
- }
-
+ /* May free signal_data */
+ remove_signal_data_if_unused (connection, signal_data);
goto out;
}
From: Simon McVittie <smcv@collabora.com>
Date: Thu, 14 Mar 2024 19:30:12 +0000
Subject: gdbusconnection: Factor out signal_data_new_take()
No functional changes, except that the implicit ownership-transfer
for the rule field becomes explicit (the local variable is set to NULL
afterwards).
Signed-off-by: Simon McVittie <smcv@collabora.com>
Origin: upstream, https://gitlab.gnome.org/GNOME/glib/-/issues/3268
---
gio/gdbusconnection.c | 42 ++++++++++++++++++++++++++++++++----------
1 file changed, 32 insertions(+), 10 deletions(-)
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index b48ed99..da08341 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -348,6 +348,30 @@ typedef struct
GPtrArray *subscribers; /* (owned) (element-type SignalSubscriber) */
} SignalData;
+static SignalData *
+signal_data_new_take (gchar *rule,
+ gchar *sender,
+ gchar *sender_unique_name,
+ gchar *interface_name,
+ gchar *member,
+ gchar *object_path,
+ gchar *arg0,
+ GDBusSignalFlags flags)
+{
+ SignalData *signal_data = g_new0 (SignalData, 1);
+
+ signal_data->rule = rule;
+ signal_data->sender = sender;
+ signal_data->sender_unique_name = sender_unique_name;
+ signal_data->interface_name = interface_name;
+ signal_data->member = member;
+ signal_data->object_path = object_path;
+ signal_data->arg0 = arg0;
+ signal_data->flags = flags;
+ signal_data->subscribers = g_ptr_array_new_with_free_func ((GDestroyNotify) signal_subscriber_unref);
+ return g_steal_pointer (&signal_data);
+}
+
static void
signal_data_free (SignalData *signal_data)
{
@@ -3573,16 +3597,14 @@ g_dbus_connection_signal_subscribe (GDBusConnection *connection,
goto out;
}
- signal_data = g_new0 (SignalData, 1);
- signal_data->rule = rule;
- signal_data->sender = g_strdup (sender);
- signal_data->sender_unique_name = g_strdup (sender_unique_name);
- signal_data->interface_name = g_strdup (interface_name);
- signal_data->member = g_strdup (member);
- signal_data->object_path = g_strdup (object_path);
- signal_data->arg0 = g_strdup (arg0);
- signal_data->flags = flags;
- signal_data->subscribers = g_ptr_array_new_with_free_func ((GDestroyNotify) signal_subscriber_unref);
+ signal_data = signal_data_new_take (g_steal_pointer (&rule),
+ g_strdup (sender),
+ g_strdup (sender_unique_name),
+ g_strdup (interface_name),
+ g_strdup (member),
+ g_strdup (object_path),
+ g_strdup (arg0),
+ flags);
g_ptr_array_add (signal_data->subscribers, subscriber);
g_hash_table_insert (connection->map_rule_to_signal_data,
From: Simon McVittie <smcv@collabora.com>
Date: Wed, 1 May 2024 15:51:42 +0100
Subject: gdbusconnection: Make a backport of g_set_str() available
A subsequent commit will need this. Copying all of g_set_str() into a
private header seems cleaner than replacing the call to it.
Helps: GNOME/glib#3268
Signed-off-by: Simon McVittie <smcv@collabora.com>
Origin: upstream, https://gitlab.gnome.org/GNOME/glib/-/issues/3268
---
gio/gdbusconnection.c | 1 +
glib/glib-private.h | 18 ++++++++++++++++++
2 files changed, 19 insertions(+)
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index 75b8180..fd6bf62 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -95,6 +95,7 @@
#include <stdlib.h>
#include <string.h>
+#include "glib-private.h"
#include "gdbusauth.h"
#include "gdbusutils.h"
#include "gdbusaddress.h"
diff --git a/glib/glib-private.h b/glib/glib-private.h
index 8b7ab13..6b9f527 100644
--- a/glib/glib-private.h
+++ b/glib/glib-private.h
@@ -105,4 +105,22 @@ GLibPrivateVTable *glib__private__ (void);
# define GLIB_DEFAULT_LOCALE ""
#endif
+/* Backported from GLib 2.78.x, where it is public API in gstrfuncs.h */
+static inline gboolean
+g_set_str (char **str_pointer,
+ const char *new_str)
+{
+ char *copy;
+
+ if (*str_pointer == new_str ||
+ (*str_pointer && new_str && strcmp (*str_pointer, new_str) == 0))
+ return FALSE;
+
+ copy = g_strdup (new_str);
+ g_free (*str_pointer);
+ *str_pointer = copy;
+
+ return TRUE;
+}
+
#endif /* __GLIB_PRIVATE_H__ */
From: Simon McVittie <smcv@collabora.com>
Date: Wed, 1 May 2024 19:43:24 +0100
Subject: gdbusconnection: Move SignalData, SignalSubscriber higher up
Subsequent changes will need to access these data structures from
on_worker_message_received(). No functional change here, only moving
code around.
[Backport to 2.66.x: fix minor conflicts]
Signed-off-by: Simon McVittie <smcv@collabora.com>
Origin: upstream, https://gitlab.gnome.org/GNOME/glib/-/issues/3268
---
gio/gdbusconnection.c | 128 +++++++++++++++++++++++++-------------------------
1 file changed, 65 insertions(+), 63 deletions(-)
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index fd6bf62..b48ed99 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -299,6 +299,71 @@ _g_strv_has_string (const gchar* const *haystack,
/* ---------------------------------------------------------------------------------------------------- */
+typedef struct
+{
+ /* All fields are immutable after construction. */
+ gatomicrefcount ref_count;
+ GDBusSignalCallback callback;
+ gpointer user_data;
+ GDestroyNotify user_data_free_func;
+ guint id;
+ GMainContext *context;
+} SignalSubscriber;
+
+static SignalSubscriber *
+signal_subscriber_ref (SignalSubscriber *subscriber)
+{
+ g_atomic_ref_count_inc (&subscriber->ref_count);
+ return subscriber;
+}
+
+static void
+signal_subscriber_unref (SignalSubscriber *subscriber)
+{
+ if (g_atomic_ref_count_dec (&subscriber->ref_count))
+ {
+ /* Destroy the user data. It doesn’t matter which thread
+ * signal_subscriber_unref() is called in (or whether it’s called with a
+ * lock held), as call_destroy_notify() always defers to the next
+ * #GMainContext iteration. */
+ call_destroy_notify (subscriber->context,
+ subscriber->user_data_free_func,
+ subscriber->user_data);
+
+ g_main_context_unref (subscriber->context);
+ g_free (subscriber);
+ }
+}
+
+typedef struct
+{
+ gchar *rule;
+ gchar *sender;
+ gchar *sender_unique_name; /* if sender is unique or org.freedesktop.DBus, then that name... otherwise blank */
+ gchar *interface_name;
+ gchar *member;
+ gchar *object_path;
+ gchar *arg0;
+ GDBusSignalFlags flags;
+ GPtrArray *subscribers; /* (owned) (element-type SignalSubscriber) */
+} SignalData;
+
+static void
+signal_data_free (SignalData *signal_data)
+{
+ g_free (signal_data->rule);
+ g_free (signal_data->sender);
+ g_free (signal_data->sender_unique_name);
+ g_free (signal_data->interface_name);
+ g_free (signal_data->member);
+ g_free (signal_data->object_path);
+ g_free (signal_data->arg0);
+ g_ptr_array_unref (signal_data->subscribers);
+ g_free (signal_data);
+}
+
+/* ---------------------------------------------------------------------------------------------------- */
+
#ifdef G_OS_WIN32
#define CONNECTION_ENSURE_LOCK(obj) do { ; } while (FALSE)
#else
@@ -3242,69 +3307,6 @@ g_dbus_connection_remove_filter (GDBusConnection *connection,
/* ---------------------------------------------------------------------------------------------------- */
-typedef struct
-{
- gchar *rule;
- gchar *sender;
- gchar *sender_unique_name; /* if sender is unique or org.freedesktop.DBus, then that name... otherwise blank */
- gchar *interface_name;
- gchar *member;
- gchar *object_path;
- gchar *arg0;
- GDBusSignalFlags flags;
- GPtrArray *subscribers; /* (owned) (element-type SignalSubscriber) */
-} SignalData;
-
-static void
-signal_data_free (SignalData *signal_data)
-{
- g_free (signal_data->rule);
- g_free (signal_data->sender);
- g_free (signal_data->sender_unique_name);
- g_free (signal_data->interface_name);
- g_free (signal_data->member);
- g_free (signal_data->object_path);
- g_free (signal_data->arg0);
- g_ptr_array_unref (signal_data->subscribers);
- g_free (signal_data);
-}
-
-typedef struct
-{
- /* All fields are immutable after construction. */
- gatomicrefcount ref_count;
- GDBusSignalCallback callback;
- gpointer user_data;
- GDestroyNotify user_data_free_func;
- guint id;
- GMainContext *context;
-} SignalSubscriber;
-
-static SignalSubscriber *
-signal_subscriber_ref (SignalSubscriber *subscriber)
-{
- g_atomic_ref_count_inc (&subscriber->ref_count);
- return subscriber;
-}
-
-static void
-signal_subscriber_unref (SignalSubscriber *subscriber)
-{
- if (g_atomic_ref_count_dec (&subscriber->ref_count))
- {
- /* Destroy the user data. It doesn’t matter which thread
- * signal_subscriber_unref() is called in (or whether it’s called with a
- * lock held), as call_destroy_notify() always defers to the next
- * #GMainContext iteration. */
- call_destroy_notify (subscriber->context,
- subscriber->user_data_free_func,
- subscriber->user_data);
-
- g_main_context_unref (subscriber->context);
- g_free (subscriber);
- }
-}
-
static gchar *
args_to_rule (const gchar *sender,
const gchar *interface_name,
From: Simon McVittie <smcv@collabora.com>
Date: Tue, 23 Apr 2024 20:39:05 +0100
Subject: gdbusconnection: Stop storing sender_unique_name in SignalData
This will become confusing when we start tracking the owner of a
well-known-name sender, and it's redundant anyway. Instead, track the
1 bit of data that we actually need: whether it's a well-known name.
Strictly speaking this too is redundant, because it's syntactically
derivable from the sender, but only via extra string operations.
A subsequent commit will add a data structure to keep track of the
owner of a well-known-name sender, at which point this boolean will
be replaced by the presence or absence of that data structure.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Origin: upstream, https://gitlab.gnome.org/GNOME/glib/-/issues/3268
---
gio/gdbusconnection.c | 36 ++++++++++++++++++++++++------------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index 821132e..ed11b4c 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -339,19 +339,19 @@ typedef struct
{
gchar *rule;
gchar *sender;
- gchar *sender_unique_name; /* if sender is unique or org.freedesktop.DBus, then that name... otherwise blank */
gchar *interface_name;
gchar *member;
gchar *object_path;
gchar *arg0;
GDBusSignalFlags flags;
GPtrArray *subscribers; /* (owned) (element-type SignalSubscriber) */
+ gboolean sender_is_its_own_owner;
} SignalData;
static SignalData *
signal_data_new_take (gchar *rule,
gchar *sender,
- gchar *sender_unique_name,
+ gboolean sender_is_its_own_owner,
gchar *interface_name,
gchar *member,
gchar *object_path,
@@ -362,7 +362,7 @@ signal_data_new_take (gchar *rule,
signal_data->rule = rule;
signal_data->sender = sender;
- signal_data->sender_unique_name = sender_unique_name;
+ signal_data->sender_is_its_own_owner = sender_is_its_own_owner;
signal_data->interface_name = interface_name;
signal_data->member = member;
signal_data->object_path = object_path;
@@ -377,7 +377,6 @@ signal_data_free (SignalData *signal_data)
{
g_free (signal_data->rule);
g_free (signal_data->sender);
- g_free (signal_data->sender_unique_name);
g_free (signal_data->interface_name);
g_free (signal_data->member);
g_free (signal_data->object_path);
@@ -3442,7 +3441,7 @@ remove_match_rule (GDBusConnection *connection,
static gboolean
is_signal_data_for_name_lost_or_acquired (SignalData *signal_data)
{
- return g_strcmp0 (signal_data->sender_unique_name, "org.freedesktop.DBus") == 0 &&
+ return g_strcmp0 (signal_data->sender, "org.freedesktop.DBus") == 0 &&
g_strcmp0 (signal_data->interface_name, "org.freedesktop.DBus") == 0 &&
g_strcmp0 (signal_data->object_path, "/org/freedesktop/DBus") == 0 &&
(g_strcmp0 (signal_data->member, "NameLost") == 0 ||
@@ -3454,7 +3453,8 @@ is_signal_data_for_name_lost_or_acquired (SignalData *signal_data)
/* called in any thread, connection lock is held */
static void
add_signal_data (GDBusConnection *connection,
- SignalData *signal_data)
+ SignalData *signal_data,
+ const char *sender_unique_name)
{
GPtrArray *signal_data_array;
@@ -3474,12 +3474,12 @@ add_signal_data (GDBusConnection *connection,
}
signal_data_array = g_hash_table_lookup (connection->map_sender_unique_name_to_signal_data_array,
- signal_data->sender_unique_name);
+ sender_unique_name);
if (signal_data_array == NULL)
{
signal_data_array = g_ptr_array_new ();
g_hash_table_insert (connection->map_sender_unique_name_to_signal_data_array,
- g_strdup (signal_data->sender_unique_name),
+ g_strdup (sender_unique_name),
signal_data_array);
}
g_ptr_array_add (signal_data_array, signal_data);
@@ -3576,6 +3576,7 @@ g_dbus_connection_signal_subscribe (GDBusConnection *connection,
gchar *rule;
SignalData *signal_data;
SignalSubscriber *subscriber;
+ gboolean sender_is_its_own_owner;
const gchar *sender_unique_name;
/* Right now we abort if AddMatch() fails since it can only fail with the bus being in
@@ -3611,6 +3612,11 @@ g_dbus_connection_signal_subscribe (GDBusConnection *connection,
rule = args_to_rule (sender, interface_name, member, object_path, arg0, flags);
if (sender != NULL && (g_dbus_is_unique_name (sender) || g_strcmp0 (sender, "org.freedesktop.DBus") == 0))
+ sender_is_its_own_owner = TRUE;
+ else
+ sender_is_its_own_owner = FALSE;
+
+ if (sender_is_its_own_owner)
sender_unique_name = sender;
else
sender_unique_name = "";
@@ -3634,14 +3640,14 @@ g_dbus_connection_signal_subscribe (GDBusConnection *connection,
signal_data = signal_data_new_take (g_steal_pointer (&rule),
g_strdup (sender),
- g_strdup (sender_unique_name),
+ sender_is_its_own_owner,
g_strdup (interface_name),
g_strdup (member),
g_strdup (object_path),
g_strdup (arg0),
flags);
g_ptr_array_add (signal_data->subscribers, subscriber);
- add_signal_data (connection, signal_data);
+ add_signal_data (connection, signal_data, sender_unique_name);
out:
g_hash_table_insert (connection->map_id_to_signal_data,
@@ -3665,22 +3671,28 @@ static void
remove_signal_data_if_unused (GDBusConnection *connection,
SignalData *signal_data)
{
+ const gchar *sender_unique_name;
GPtrArray *signal_data_array;
if (signal_data->subscribers->len != 0)
return;
+ if (signal_data->sender_is_its_own_owner)
+ sender_unique_name = signal_data->sender;
+ else
+ sender_unique_name = "";
+
g_warn_if_fail (g_hash_table_remove (connection->map_rule_to_signal_data, signal_data->rule));
signal_data_array = g_hash_table_lookup (connection->map_sender_unique_name_to_signal_data_array,
- signal_data->sender_unique_name);
+ sender_unique_name);
g_warn_if_fail (signal_data_array != NULL);
g_warn_if_fail (g_ptr_array_remove (signal_data_array, signal_data));
if (signal_data_array->len == 0)
{
g_warn_if_fail (g_hash_table_remove (connection->map_sender_unique_name_to_signal_data_array,
- signal_data->sender_unique_name));
+ sender_unique_name));
}
/* remove the match rule from the bus unless NameLost or NameAcquired (see subscribe()) */
From: Philip Withnall <pwithnall@gnome.org>
Date: Tue, 28 Nov 2023 12:58:20 +0000
Subject: gdbusmessage: Cache the arg0 value
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
Technically we can’t rely on it being kept alive by the `message->body`
pointer, unless we can guarantee that the `GVariant` is always
serialised. That’s not necessarily the case, so keep a separate ref on
the arg0 value at all times.
This avoids a potential use-after-free.
Spotted by Thomas Haller in
https://gitlab.gnome.org/GNOME/glib/-/merge_requests/3720#note_1924707.
[This is a prerequisite for having tests pass after fixing the
vulnerability described in glib#3268, because after fixing that
vulnerability, the use-after-free genuinely does happen during
regression testing. -smcv]
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3183, #3268
(cherry picked from commit 10e9a917be7fb92b6b27837ef7a7f1d0be6095d5)
Origin: upstream, commit:https://gitlab.gnome.org/GNOME/glib/-/commit/10e9a917be7fb92b6b27837ef7a7f1d0be6095d5
---
gio/gdbusmessage.c | 35 ++++++++++++++++++++++-------------
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/gio/gdbusmessage.c b/gio/gdbusmessage.c
index 0e4878d..c4357cb 100644
--- a/gio/gdbusmessage.c
+++ b/gio/gdbusmessage.c
@@ -471,6 +471,7 @@ struct _GDBusMessage
guint32 serial;
GHashTable *headers;
GVariant *body;
+ GVariant *arg0_cache; /* (nullable) (owned) */
#ifdef G_OS_UNIX
GUnixFDList *fd_list;
#endif
@@ -493,6 +494,7 @@ g_dbus_message_finalize (GObject *object)
g_hash_table_unref (message->headers);
if (message->body != NULL)
g_variant_unref (message->body);
+ g_clear_pointer (&message->arg0_cache, g_variant_unref);
#ifdef G_OS_UNIX
if (message->fd_list != NULL)
g_object_unref (message->fd_list);
@@ -1128,6 +1130,7 @@ g_dbus_message_set_body (GDBusMessage *message,
if (body == NULL)
{
message->body = NULL;
+ message->arg0_cache = NULL;
g_dbus_message_set_signature (message, NULL);
}
else
@@ -1138,6 +1141,12 @@ g_dbus_message_set_body (GDBusMessage *message,
message->body = g_variant_ref_sink (body);
+ if (g_variant_is_of_type (message->body, G_VARIANT_TYPE_TUPLE) &&
+ g_variant_n_children (message->body) > 0)
+ message->arg0_cache = g_variant_get_child_value (message->body, 0);
+ else
+ message->arg0_cache = NULL;
+
type_string = g_variant_get_type_string (body);
type_string_len = strlen (type_string);
g_assert (type_string_len >= 2);
@@ -2219,6 +2228,14 @@ g_dbus_message_new_from_blob (guchar *blob,
2,
error);
g_variant_type_free (variant_type);
+
+ if (message->body != NULL &&
+ g_variant_is_of_type (message->body, G_VARIANT_TYPE_TUPLE) &&
+ g_variant_n_children (message->body) > 0)
+ message->arg0_cache = g_variant_get_child_value (message->body, 0);
+ else
+ message->arg0_cache = NULL;
+
if (message->body == NULL)
goto out;
}
@@ -3254,22 +3271,13 @@ g_dbus_message_set_signature (GDBusMessage *message,
const gchar *
g_dbus_message_get_arg0 (GDBusMessage *message)
{
- const gchar *ret;
-
g_return_val_if_fail (G_IS_DBUS_MESSAGE (message), NULL);
- ret = NULL;
+ if (message->arg0_cache != NULL &&
+ g_variant_is_of_type (message->arg0_cache, G_VARIANT_TYPE_STRING))
+ return g_variant_get_string (message->arg0_cache, NULL);
- if (message->body != NULL && g_variant_is_of_type (message->body, G_VARIANT_TYPE_TUPLE))
- {
- GVariant *item;
- item = g_variant_get_child_value (message->body, 0);
- if (g_variant_is_of_type (item, G_VARIANT_TYPE_STRING))
- ret = g_variant_get_string (item, NULL);
- g_variant_unref (item);
- }
-
- return ret;
+ return NULL;
}
/* ---------------------------------------------------------------------------------------------------- */
@@ -3712,6 +3720,7 @@ g_dbus_message_copy (GDBusMessage *message,
* to just ref (as opposed to deep-copying) the GVariant instances
*/
ret->body = message->body != NULL ? g_variant_ref (message->body) : NULL;
+ ret->arg0_cache = message->arg0_cache != NULL ? g_variant_ref (message->arg0_cache) : NULL;
g_hash_table_iter_init (&iter, message->headers);
while (g_hash_table_iter_next (&iter, &header_key, (gpointer) &header_value))
g_hash_table_insert (ret->headers, header_key, g_variant_ref (header_value));
From: Simon McVittie <smcv@collabora.com>
Date: Thu, 14 Mar 2024 19:18:15 +0000
Subject: gdbusprivate: Add symbolic constants for the message bus itself
Using these is a bit more clearly correct than repeating them everywhere.
To avoid excessive diffstat in a branch for a bug fix, I'm not
immediately replacing all existing occurrences of the same literals with
these names.
The names of these constants are chosen to be consistent with libdbus,
despite using somewhat outdated terminology (D-Bus now uses the term
"well-known bus name" for what used to be called a service name,
reserving the word "service" to mean specifically the programs that
have .service files and participate in service activation).
Signed-off-by: Simon McVittie <smcv@collabora.com>
Origin: upstream, https://gitlab.gnome.org/GNOME/glib/-/issues/3268
---
gio/gdbusprivate.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/gio/gdbusprivate.h b/gio/gdbusprivate.h
index 8f8a93b..e9bca11 100644
--- a/gio/gdbusprivate.h
+++ b/gio/gdbusprivate.h
@@ -29,6 +29,11 @@
G_BEGIN_DECLS
+/* Bus name, interface and object path of the message bus itself */
+#define DBUS_SERVICE_DBUS "org.freedesktop.DBus"
+#define DBUS_INTERFACE_DBUS DBUS_SERVICE_DBUS
+#define DBUS_PATH_DBUS "/org/freedesktop/DBus"
+
/* ---------------------------------------------------------------------------------------------------- */
typedef struct GDBusWorker GDBusWorker;
From: Simon McVittie <smcv@collabora.com>
Date: Fri, 8 Mar 2024 19:44:03 +0000
Subject: tests: Add a test-case for what happens if a unique name doesn't
exist
On GNOME/glib#3268 there was some concern about whether this would
allow an attacker to send signals and have them be matched to a
GDBusProxy in this situation, but it seems that was a false alarm.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Origin: upstream, https://gitlab.gnome.org/GNOME/glib/-/issues/3268
---
gio/tests/gdbus-subscribe.c | 48 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/gio/tests/gdbus-subscribe.c b/gio/tests/gdbus-subscribe.c
index 3d2a14e..350ec9f 100644
--- a/gio/tests/gdbus-subscribe.c
+++ b/gio/tests/gdbus-subscribe.c
@@ -358,6 +358,53 @@ static const TestPlan plan_limit_by_unique_name =
},
};
+static const TestPlan plan_nonexistent_unique_name =
+{
+ .description = "A subscription via a unique name that doesn't exist "
+ "accepts no messages",
+ .steps = {
+ {
+ /* Subscriber wants to receive signals from service */
+ .action = TEST_ACTION_SUBSCRIBE,
+ .u.subscribe = {
+ /* This relies on the implementation detail that the dbus-daemon
+ * (and presumably other bus implementations) never actually generates
+ * a unique name in this format */
+ .string_sender = ":0.this.had.better.not.exist",
+ .path = EXAMPLE_PATH,
+ .iface = EXAMPLE_INTERFACE,
+ },
+ },
+ {
+ /* Attacker wants to trick subscriber into thinking that service
+ * sent a signal */
+ .action = TEST_ACTION_EMIT_SIGNAL,
+ .u.signal = {
+ .sender = TEST_CONN_ATTACKER,
+ .path = EXAMPLE_PATH,
+ .iface = EXAMPLE_INTERFACE,
+ .member = FOO_SIGNAL,
+ .received_by_conn = 0,
+ .received_by_proxy = 0
+ },
+ },
+ {
+ /* Attacker tries harder, by sending a signal unicast directly to
+ * the subscriber */
+ .action = TEST_ACTION_EMIT_SIGNAL,
+ .u.signal = {
+ .sender = TEST_CONN_ATTACKER,
+ .unicast_to = TEST_CONN_SUBSCRIBER,
+ .path = EXAMPLE_PATH,
+ .iface = EXAMPLE_INTERFACE,
+ .member = FOO_SIGNAL,
+ .received_by_conn = 0,
+ .received_by_proxy = 0
+ },
+ },
+ },
+};
+
static const TestPlan plan_limit_by_well_known_name =
{
.description = "A subscription via a well-known name only accepts messages "
@@ -1051,6 +1098,7 @@ main (int argc,
ADD_SUBSCRIBE_TEST (broadcast_from_anyone);
ADD_SUBSCRIBE_TEST (match_twice);
ADD_SUBSCRIBE_TEST (limit_by_unique_name);
+ ADD_SUBSCRIBE_TEST (nonexistent_unique_name);
ADD_SUBSCRIBE_TEST (limit_by_well_known_name);
return g_test_run();
From: Simon McVittie <smcv@collabora.com>
Date: Fri, 8 Mar 2024 19:51:50 +0000
Subject: tests: Add a test for matching by two well-known names
The expected result is that because TEST_CONN_SERVICE owns
ALREADY_OWNED_NAME but not (yet) OWNED_LATER_NAME, the signal will be
delivered to the subscriber for the former but not the latter.
Before #3268 was fixed, it was incorrectly delivered to both.
Reproduces: https://gitlab.gnome.org/GNOME/glib/-/issues/3268 (partially)
Signed-off-by: Simon McVittie <smcv@collabora.com>
Origin: upstream, https://gitlab.gnome.org/GNOME/glib/-/issues/3268
---
gio/tests/gdbus-subscribe.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/gio/tests/gdbus-subscribe.c b/gio/tests/gdbus-subscribe.c
index af100de..171d610 100644
--- a/gio/tests/gdbus-subscribe.c
+++ b/gio/tests/gdbus-subscribe.c
@@ -440,6 +440,19 @@ static const TestPlan plan_limit_by_well_known_name =
.iface = EXAMPLE_INTERFACE,
},
},
+ {
+ /* When the service sends a signal with the name it already owns,
+ * it should get through */
+ .action = TEST_ACTION_EMIT_SIGNAL,
+ .u.signal = {
+ .sender = TEST_CONN_SERVICE,
+ .path = EXAMPLE_PATH,
+ .iface = EXAMPLE_INTERFACE,
+ .member = FOO_SIGNAL,
+ .received_by_conn = 1,
+ .received_by_proxy = 1
+ },
+ },
{
/* Service claims another name */
.action = TEST_ACTION_OWN_NAME,
From: Simon McVittie <smcv@collabora.com>
Date: Fri, 8 Mar 2024 19:53:22 +0000
Subject: tests: Add a test for signal filtering by well-known name
The vulnerability reported as GNOME/glib#3268 can be characterized
as: these signals from an attacker should not be delivered to either
the GDBusConnection or the GDBusProxy, but in fact they are (in at
least some scenarios).
Reproduces: https://gitlab.gnome.org/GNOME/glib/-/issues/3268
Signed-off-by: Simon McVittie <smcv@collabora.com>
Origin: upstream, https://gitlab.gnome.org/GNOME/glib/-/issues/3268
---
gio/tests/gdbus-subscribe.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/gio/tests/gdbus-subscribe.c b/gio/tests/gdbus-subscribe.c
index 171d610..5406ba7 100644
--- a/gio/tests/gdbus-subscribe.c
+++ b/gio/tests/gdbus-subscribe.c
@@ -440,6 +440,33 @@ static const TestPlan plan_limit_by_well_known_name =
.iface = EXAMPLE_INTERFACE,
},
},
+ {
+ /* Attacker wants to trick subscriber into thinking that service
+ * sent a signal */
+ .action = TEST_ACTION_EMIT_SIGNAL,
+ .u.signal = {
+ .sender = TEST_CONN_ATTACKER,
+ .path = EXAMPLE_PATH,
+ .iface = EXAMPLE_INTERFACE,
+ .member = FOO_SIGNAL,
+ .received_by_conn = 0,
+ .received_by_proxy = 0
+ },
+ },
+ {
+ /* Attacker tries harder, by sending a signal unicast directly to
+ * the subscriber */
+ .action = TEST_ACTION_EMIT_SIGNAL,
+ .u.signal = {
+ .sender = TEST_CONN_ATTACKER,
+ .unicast_to = TEST_CONN_SUBSCRIBER,
+ .path = EXAMPLE_PATH,
+ .iface = EXAMPLE_INTERFACE,
+ .member = FOO_SIGNAL,
+ .received_by_conn = 0,
+ .received_by_proxy = 0
+ },
+ },
{
/* When the service sends a signal with the name it already owns,
* it should get through */
From: Simon McVittie <smcv@collabora.com>
Date: Fri, 8 Mar 2024 19:28:15 +0000
Subject: tests: Add support for subscribing to signals from a well-known name
Signed-off-by: Simon McVittie <smcv@collabora.com>
Origin: upstream, https://gitlab.gnome.org/GNOME/glib/-/issues/3268
---
gio/tests/gdbus-subscribe.c | 133 +++++++++++++++++++++++++++++++++++++++++---
1 file changed, 126 insertions(+), 7 deletions(-)
diff --git a/gio/tests/gdbus-subscribe.c b/gio/tests/gdbus-subscribe.c
index 3f53e1d..3d2a14e 100644
--- a/gio/tests/gdbus-subscribe.c
+++ b/gio/tests/gdbus-subscribe.c
@@ -7,6 +7,9 @@
#include "gdbus-tests.h"
+/* From the D-Bus Specification */
+#define DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER 1
+
#define DBUS_SERVICE_DBUS "org.freedesktop.DBus"
#define DBUS_PATH_DBUS "/org/freedesktop/DBus"
#define DBUS_INTERFACE_DBUS DBUS_SERVICE_DBUS
@@ -22,6 +25,9 @@
#define EXAMPLE_INTERFACE "org.gtk.GDBus.ExampleInterface"
#define FOO_SIGNAL "Foo"
+#define ALREADY_OWNED_NAME "org.gtk.Test.AlreadyOwned"
+#define OWNED_LATER_NAME "org.gtk.Test.OwnedLater"
+
/* Log @s in a debug message. */
static inline const char *
nonnull (const char *s,
@@ -101,7 +107,8 @@ typedef struct
typedef struct
{
- TestConn sender;
+ const char *string_sender;
+ TestConn unique_sender;
const char *path;
const char *iface;
const char *member;
@@ -109,11 +116,18 @@ typedef struct
GDBusSignalFlags flags;
} TestSubscribe;
+typedef struct
+{
+ const char *name;
+ TestConn owner;
+} TestOwnName;
+
typedef enum
{
TEST_ACTION_NONE = 0,
TEST_ACTION_SUBSCRIBE,
TEST_ACTION_EMIT_SIGNAL,
+ TEST_ACTION_OWN_NAME,
} TestAction;
typedef struct
@@ -122,6 +136,7 @@ typedef struct
union {
TestEmitSignal signal;
TestSubscribe subscribe;
+ TestOwnName own_name;
} u;
} TestStep;
@@ -247,7 +262,7 @@ static const TestPlan plan_match_twice =
{
.action = TEST_ACTION_SUBSCRIBE,
.u.subscribe = {
- .sender = TEST_CONN_SERVICE,
+ .unique_sender = TEST_CONN_SERVICE,
.path = EXAMPLE_PATH,
.iface = EXAMPLE_INTERFACE,
},
@@ -267,7 +282,7 @@ static const TestPlan plan_match_twice =
{
.action = TEST_ACTION_SUBSCRIBE,
.u.subscribe = {
- .sender = TEST_CONN_SERVICE,
+ .unique_sender = TEST_CONN_SERVICE,
.path = EXAMPLE_PATH,
.iface = EXAMPLE_INTERFACE,
},
@@ -296,7 +311,7 @@ static const TestPlan plan_limit_by_unique_name =
/* Subscriber wants to receive signals from service */
.action = TEST_ACTION_SUBSCRIBE,
.u.subscribe = {
- .sender = TEST_CONN_SERVICE,
+ .unique_sender = TEST_CONN_SERVICE,
.path = EXAMPLE_PATH,
.iface = EXAMPLE_INTERFACE,
},
@@ -343,6 +358,62 @@ static const TestPlan plan_limit_by_unique_name =
},
};
+static const TestPlan plan_limit_by_well_known_name =
+{
+ .description = "A subscription via a well-known name only accepts messages "
+ "sent by the owner of that well-known name",
+ .steps = {
+ {
+ /* Service already owns one name */
+ .action = TEST_ACTION_OWN_NAME,
+ .u.own_name = {
+ .name = ALREADY_OWNED_NAME,
+ .owner = TEST_CONN_SERVICE
+ },
+ },
+ {
+ /* Subscriber wants to receive signals from service */
+ .action = TEST_ACTION_SUBSCRIBE,
+ .u.subscribe = {
+ .string_sender = ALREADY_OWNED_NAME,
+ .path = EXAMPLE_PATH,
+ .iface = EXAMPLE_INTERFACE,
+ },
+ },
+ {
+ /* Subscriber wants to receive signals from service by another name */
+ .action = TEST_ACTION_SUBSCRIBE,
+ .u.subscribe = {
+ .string_sender = OWNED_LATER_NAME,
+ .path = EXAMPLE_PATH,
+ .iface = EXAMPLE_INTERFACE,
+ },
+ },
+ {
+ /* Service claims another name */
+ .action = TEST_ACTION_OWN_NAME,
+ .u.own_name = {
+ .name = OWNED_LATER_NAME,
+ .owner = TEST_CONN_SERVICE
+ },
+ },
+ {
+ /* Now the subscriber gets this signal twice, once for each
+ * subscription; and similarly each of the two proxies gets this
+ * signal twice */
+ .action = TEST_ACTION_EMIT_SIGNAL,
+ .u.signal = {
+ .sender = TEST_CONN_SERVICE,
+ .path = EXAMPLE_PATH,
+ .iface = EXAMPLE_INTERFACE,
+ .member = FOO_SIGNAL,
+ .received_by_conn = 2,
+ .received_by_proxy = 2
+ },
+ },
+ },
+};
+
typedef struct
{
const TestPlan *plan;
@@ -540,11 +611,16 @@ fixture_subscribe (Fixture *f,
GDBusConnection *subscriber = f->conns[TEST_CONN_SUBSCRIBER];
const char *sender;
- if (subscribe->sender != TEST_CONN_NONE)
+ if (subscribe->string_sender != NULL)
+ {
+ sender = subscribe->string_sender;
+ g_test_message ("\tSender: %s", sender);
+ }
+ else if (subscribe->unique_sender != TEST_CONN_NONE)
{
- sender = f->unique_names[subscribe->sender];
+ sender = f->unique_names[subscribe->unique_sender];
g_test_message ("\tSender: %s %s",
- test_conn_descriptions[subscribe->sender],
+ test_conn_descriptions[subscribe->unique_sender],
sender);
}
else
@@ -689,6 +765,43 @@ fixture_emit_signal (Fixture *f,
connection_wait_for_bus (f->conns[signal->sender]);
}
+static void
+fixture_own_name (Fixture *f,
+ const TestOwnName *own_name)
+{
+ GVariant *call_result;
+ guint32 flags;
+ guint32 result_code;
+
+ g_test_message ("\tName: %s", own_name->name);
+ g_test_message ("\tOwner: %s",
+ test_conn_descriptions[own_name->owner]);
+
+ /* For simplicity, we do this via a direct bus call rather than
+ * using g_bus_own_name_on_connection(). The flags in
+ * GBusNameOwnerFlags are numerically equal to those in the
+ * D-Bus wire protocol. */
+ flags = G_BUS_NAME_OWNER_FLAGS_DO_NOT_QUEUE;
+ call_result = g_dbus_connection_call_sync (f->conns[own_name->owner],
+ DBUS_SERVICE_DBUS,
+ DBUS_PATH_DBUS,
+ DBUS_INTERFACE_DBUS,
+ "RequestName",
+ g_variant_new ("(su)",
+ own_name->name,
+ flags),
+ G_VARIANT_TYPE ("(u)"),
+ G_DBUS_CALL_FLAGS_NONE,
+ -1,
+ NULL,
+ &f->error);
+ g_assert_no_error (f->error);
+ g_assert_nonnull (call_result);
+ g_variant_get (call_result, "(u)", &result_code);
+ g_assert_cmpuint (result_code, ==, DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER);
+ g_variant_unref (call_result);
+}
+
static void
fixture_run_plan (Fixture *f,
const TestPlan *plan,
@@ -721,6 +834,11 @@ fixture_run_plan (Fixture *f,
fixture_emit_signal (f, &step->u.signal, i);
break;
+ case TEST_ACTION_OWN_NAME:
+ g_test_message ("Step %u: claiming bus name", i);
+ fixture_own_name (f, &step->u.own_name);
+ break;
+
case TEST_ACTION_NONE:
/* Padding to fill the rest of the array, do nothing */
break;
@@ -933,6 +1051,7 @@ main (int argc,
ADD_SUBSCRIBE_TEST (broadcast_from_anyone);
ADD_SUBSCRIBE_TEST (match_twice);
ADD_SUBSCRIBE_TEST (limit_by_unique_name);
+ ADD_SUBSCRIBE_TEST (limit_by_well_known_name);
return g_test_run();
}
From: Simon McVittie <smcv@collabora.com>
Date: Fri, 8 Mar 2024 20:10:29 +0000
Subject: tests: Add test coverage for signals that match the message bus's
name
This is a special case of unique names, even though it's syntactically
a well-known name.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Origin: upstream, https://gitlab.gnome.org/GNOME/glib/-/issues/3268
---
gio/tests/gdbus-subscribe.c | 161 ++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 154 insertions(+), 7 deletions(-)
diff --git a/gio/tests/gdbus-subscribe.c b/gio/tests/gdbus-subscribe.c
index 350ec9f..af100de 100644
--- a/gio/tests/gdbus-subscribe.c
+++ b/gio/tests/gdbus-subscribe.c
@@ -13,6 +13,7 @@
#define DBUS_SERVICE_DBUS "org.freedesktop.DBus"
#define DBUS_PATH_DBUS "/org/freedesktop/DBus"
#define DBUS_INTERFACE_DBUS DBUS_SERVICE_DBUS
+#define NAME_OWNER_CHANGED "NameOwnerChanged"
/* A signal that each connection emits to indicate that it has finished
* emitting other signals */
@@ -101,6 +102,7 @@ typedef struct
const char *iface;
const char *member;
const char *arg0;
+ const char *args;
guint received_by_conn;
guint received_by_proxy;
} TestEmitSignal;
@@ -120,6 +122,8 @@ typedef struct
{
const char *name;
TestConn owner;
+ guint received_by_conn;
+ guint received_by_proxy;
} TestOwnName;
typedef enum
@@ -461,6 +465,63 @@ static const TestPlan plan_limit_by_well_known_name =
},
};
+static const TestPlan plan_limit_to_message_bus =
+{
+ .description = "A subscription to the message bus only accepts messages "
+ "from the message bus",
+ .steps = {
+ {
+ /* Subscriber wants to receive signals from the message bus itself */
+ .action = TEST_ACTION_SUBSCRIBE,
+ .u.subscribe = {
+ .string_sender = DBUS_SERVICE_DBUS,
+ .path = DBUS_PATH_DBUS,
+ .iface = DBUS_INTERFACE_DBUS,
+ },
+ },
+ {
+ /* Attacker wants to trick subscriber into thinking that the message
+ * bus sent a signal */
+ .action = TEST_ACTION_EMIT_SIGNAL,
+ .u.signal = {
+ .sender = TEST_CONN_ATTACKER,
+ .path = DBUS_PATH_DBUS,
+ .iface = DBUS_INTERFACE_DBUS,
+ .member = NAME_OWNER_CHANGED,
+ .arg0 = "would I lie to you?",
+ .received_by_conn = 0,
+ .received_by_proxy = 0
+ },
+ },
+ {
+ /* Attacker tries harder, by sending a signal unicast directly to
+ * the subscriber, and using more realistic arguments */
+ .action = TEST_ACTION_EMIT_SIGNAL,
+ .u.signal = {
+ .unicast_to = TEST_CONN_SUBSCRIBER,
+ .sender = TEST_CONN_ATTACKER,
+ .path = DBUS_PATH_DBUS,
+ .iface = DBUS_INTERFACE_DBUS,
+ .member = NAME_OWNER_CHANGED,
+ .args = "('com.example.Name', '', ':1.12')",
+ .received_by_conn = 0,
+ .received_by_proxy = 0
+ },
+ },
+ {
+ /* When the message bus sends a signal (in this case triggered by
+ * owning a name), it should still get through */
+ .action = TEST_ACTION_OWN_NAME,
+ .u.own_name = {
+ .name = OWNED_LATER_NAME,
+ .owner = TEST_CONN_SERVICE,
+ .received_by_conn = 1,
+ .received_by_proxy = 1
+ },
+ },
+ },
+};
+
typedef struct
{
const TestPlan *plan;
@@ -591,7 +652,18 @@ fixture_received_signal (Fixture *f,
}
}
- g_assert_cmpint (received->sender, !=, TEST_CONN_NONE);
+ if (g_str_equal (sender_name, DBUS_SERVICE_DBUS))
+ {
+ g_test_message ("Signal received from message bus %s",
+ sender_name);
+ }
+ else
+ {
+ g_test_message ("Signal received from %s %s",
+ test_conn_descriptions[received->sender],
+ sender_name);
+ g_assert_cmpint (received->sender, !=, TEST_CONN_NONE);
+ }
g_test_message ("Signal received from %s %s via %s",
test_conn_descriptions[received->sender],
@@ -607,13 +679,56 @@ fixture_received_signal (Fixture *f,
g_test_message ("\tString argument 0: %s", received->arg0);
g_test_message ("\tSent in step: %u", received->step);
}
- else
+ else if (g_variant_is_of_type (parameters, G_VARIANT_TYPE ("(uu)")))
{
- g_assert_cmpstr (g_variant_get_type_string (parameters), ==, "(uu)");
g_variant_get (parameters, "(uu)", NULL, &received->step);
g_test_message ("\tArgument 0: (not a string)");
g_test_message ("\tSent in step: %u", received->step);
}
+ else if (g_variant_is_of_type (parameters, G_VARIANT_TYPE ("(sss)")))
+ {
+ const char *name;
+ const char *old_owner;
+ const char *new_owner;
+
+ /* The only signal of this signature that we legitimately receive
+ * during this test is NameOwnerChanged, so just assert that it
+ * is from the message bus and can be matched to a plausible step.
+ * (This is less thorough than the above, and will not work if we
+ * add a test scenario where a name's ownership is repeatedly
+ * changed while watching NameOwnerChanged - so don't do that.) */
+ g_assert_cmpstr (sender_name, ==, DBUS_SERVICE_DBUS);
+ g_assert_cmpstr (path, ==, DBUS_PATH_DBUS);
+ g_assert_cmpstr (iface, ==, DBUS_INTERFACE_DBUS);
+ g_assert_cmpstr (member, ==, NAME_OWNER_CHANGED);
+
+ g_variant_get (parameters, "(&s&s&s)", &name, &old_owner, &new_owner);
+
+ for (i = 0; i < G_N_ELEMENTS (f->plan->steps); i++)
+ {
+ const TestStep *step = &f->plan->steps[i];
+
+ if (step->action == TEST_ACTION_OWN_NAME)
+ {
+ const TestOwnName *own_name = &step->u.own_name;
+
+ if (g_str_equal (name, own_name->name)
+ && g_str_equal (new_owner, f->unique_names[own_name->owner])
+ && own_name->received_by_conn > 0)
+ {
+ received->step = i;
+ break;
+ }
+ }
+
+ if (i >= G_N_ELEMENTS (f->plan->steps))
+ g_error ("Could not match message to a test step");
+ }
+ }
+ else
+ {
+ g_error ("Unexpected message received");
+ }
g_ptr_array_add (f->received, g_steal_pointer (&received));
}
@@ -782,10 +897,15 @@ fixture_emit_signal (Fixture *f,
* Otherwise put something that will not match any arg0.
* Either way, put the sequence number in argument 1 so we can
* correlate sent messages with received messages later. */
- if (signal->arg0 != NULL)
+ if (signal->args != NULL)
{
- g_test_message ("\tString argument 0: %s", signal->arg0);
/* floating */
+ body = g_variant_new_parsed (signal->args);
+ g_assert_nonnull (body);
+ }
+ else if (signal->arg0 != NULL)
+ {
+ g_test_message ("\tString argument 0: %s", signal->arg0);
body = g_variant_new ("(su)", signal->arg0, (guint32) step_number);
}
else
@@ -933,8 +1053,6 @@ fixture_run_plan (Fixture *f,
g_assert_cmpuint (received->step, <, G_N_ELEMENTS (f->received_by_conn));
g_assert_cmpuint (received->step, <, G_N_ELEMENTS (f->received_by_proxy));
- g_assert_cmpint (plan->steps[received->step].action,
- ==, TEST_ACTION_EMIT_SIGNAL);
if (received->received_by_proxy != NULL)
f->received_by_proxy[received->step] += 1;
@@ -974,6 +1092,34 @@ fixture_run_plan (Fixture *f,
g_assert_cmpuint (f->received_by_proxy[i], ==, 0);
}
}
+ else if (step->action == TEST_ACTION_OWN_NAME)
+ {
+ const TestOwnName *own_name = &plan->steps[i].u.own_name;
+
+ if (mode != SUBSCRIPTION_MODE_PROXY)
+ {
+ g_test_message ("NameOwnerChanged from step %u was received %u "
+ "times by GDBusConnection, expected %u",
+ i, f->received_by_conn[i], own_name->received_by_conn);
+ g_assert_cmpuint (f->received_by_conn[i], ==, own_name->received_by_conn);
+ }
+ else
+ {
+ g_assert_cmpuint (f->received_by_conn[i], ==, 0);
+ }
+
+ if (mode != SUBSCRIPTION_MODE_CONN)
+ {
+ g_test_message ("NameOwnerChanged from step %u was received %u "
+ "times by GDBusProxy, expected %u",
+ i, f->received_by_proxy[i], own_name->received_by_proxy);
+ g_assert_cmpuint (f->received_by_proxy[i], ==, own_name->received_by_proxy);
+ }
+ else
+ {
+ g_assert_cmpuint (f->received_by_proxy[i], ==, 0);
+ }
+ }
}
}
@@ -1100,6 +1246,7 @@ main (int argc,
ADD_SUBSCRIBE_TEST (limit_by_unique_name);
ADD_SUBSCRIBE_TEST (nonexistent_unique_name);
ADD_SUBSCRIBE_TEST (limit_by_well_known_name);
+ ADD_SUBSCRIBE_TEST (limit_to_message_bus);
return g_test_run();
}
From: Simon McVittie <smcv@collabora.com>
Date: Tue, 23 Apr 2024 21:39:43 +0100
Subject: tests: Ensure that unsubscribing with GetNameOwner in-flight doesn't
crash
This was a bug that existed during development of this branch; make sure
it doesn't come back.
This test fails with a use-after-free and crash if we comment out the
part of name_watcher_unref_watched_name() that removes the name watcher
from `map_method_serial_to_name_watcher`.
It would also fail with an assertion failure if we asserted in
name_watcher_unref_watched_name() that get_name_owner_serial == 0
(i.e. that GetNameOwner is not in-flight at destruction).
Signed-off-by: Simon McVittie <smcv@collabora.com>
Origin: upstream, https://gitlab.gnome.org/GNOME/glib/-/issues/3268
---
gio/tests/gdbus-subscribe.c | 52 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 51 insertions(+), 1 deletion(-)
diff --git a/gio/tests/gdbus-subscribe.c b/gio/tests/gdbus-subscribe.c
index 5406ba7..4cba4f5 100644
--- a/gio/tests/gdbus-subscribe.c
+++ b/gio/tests/gdbus-subscribe.c
@@ -116,6 +116,7 @@ typedef struct
const char *member;
const char *arg0;
GDBusSignalFlags flags;
+ gboolean unsubscribe_immediately;
} TestSubscribe;
typedef struct
@@ -141,6 +142,7 @@ typedef struct
TestEmitSignal signal;
TestSubscribe subscribe;
TestOwnName own_name;
+ guint unsubscribe_undo_step;
} u;
} TestStep;
@@ -505,6 +507,43 @@ static const TestPlan plan_limit_by_well_known_name =
},
};
+static const TestPlan plan_unsubscribe_immediately =
+{
+ .description = "Unsubscribing before GetNameOwner can return doesn't result in a crash",
+ .steps = {
+ {
+ /* Service already owns one name */
+ .action = TEST_ACTION_OWN_NAME,
+ .u.own_name = {
+ .name = ALREADY_OWNED_NAME,
+ .owner = TEST_CONN_SERVICE
+ },
+ },
+ {
+ .action = TEST_ACTION_SUBSCRIBE,
+ .u.subscribe = {
+ .string_sender = ALREADY_OWNED_NAME,
+ .path = EXAMPLE_PATH,
+ .iface = EXAMPLE_INTERFACE,
+ .unsubscribe_immediately = TRUE
+ },
+ },
+ {
+ .action = TEST_ACTION_EMIT_SIGNAL,
+ .u.signal = {
+ .sender = TEST_CONN_SERVICE,
+ .path = EXAMPLE_PATH,
+ .iface = EXAMPLE_INTERFACE,
+ .member = FOO_SIGNAL,
+ .received_by_conn = 0,
+ /* The proxy can't unsubscribe, except by destroying the proxy
+ * completely, which we don't currently implement in this test */
+ .received_by_proxy = 1
+ },
+ },
+ },
+};
+
static const TestPlan plan_limit_to_message_bus =
{
.description = "A subscription to the message bus only accepts messages "
@@ -855,8 +894,18 @@ fixture_subscribe (Fixture *f,
subscribe->flags,
subscribed_signal_cb,
f, NULL);
+
g_assert_cmpuint (id, !=, 0);
- f->subscriptions[step_number] = id;
+
+ if (subscribe->unsubscribe_immediately)
+ {
+ g_test_message ("\tImmediately unsubscribing");
+ g_dbus_connection_signal_unsubscribe (subscriber, id);
+ }
+ else
+ {
+ f->subscriptions[step_number] = id;
+ }
}
if (f->mode != SUBSCRIPTION_MODE_CONN)
@@ -1287,6 +1336,7 @@ main (int argc,
ADD_SUBSCRIBE_TEST (nonexistent_unique_name);
ADD_SUBSCRIBE_TEST (limit_by_well_known_name);
ADD_SUBSCRIBE_TEST (limit_to_message_bus);
+ ADD_SUBSCRIBE_TEST (unsubscribe_immediately);
return g_test_run();
}
......@@ -54,3 +54,21 @@ debian/gvariant-test-Don-t-run-at-build-time-on-mips.patch
debian/taptestrunner-Stop-looking-like-an-executable-script.patch
debian/testfilemonitor-Skip-if-we-are-avoiding-flaky-tests.patch
debian/gdbus-server-auth-Normally-skip-flaky-DBUS_COOKIE_SHA1-te.patch
CVE-2024-34397/gdbusmessage-Cache-the-arg0-value.patch
CVE-2024-34397/gdbusconnection-Make-a-backport-of-g_set_str-available.patch
CVE-2024-34397/tests-Add-a-data-driven-test-for-signal-subscriptions.patch
CVE-2024-34397/tests-Add-support-for-subscribing-to-signals-from-a-well-.patch
CVE-2024-34397/tests-Add-a-test-case-for-what-happens-if-a-unique-name-d.patch
CVE-2024-34397/tests-Add-test-coverage-for-signals-that-match-the-messag.patch
CVE-2024-34397/gdbusprivate-Add-symbolic-constants-for-the-message-bus-i.patch
CVE-2024-34397/gdbusconnection-Move-SignalData-SignalSubscriber-higher-u.patch
CVE-2024-34397/gdbusconnection-Factor-out-signal_data_new_take.patch
CVE-2024-34397/gdbusconnection-Factor-out-add_signal_data.patch
CVE-2024-34397/gdbusconnection-Factor-out-remove_signal_data_if_unused.patch
CVE-2024-34397/gdbusconnection-Stop-storing-sender_unique_name-in-Signal.patch
CVE-2024-34397/gdbus-Track-name-owners-for-signal-subscriptions.patch
CVE-2024-34397/gdbusconnection-Don-t-deliver-signals-if-the-sender-doesn.patch
CVE-2024-34397/tests-Add-a-test-for-matching-by-two-well-known-names.patch
CVE-2024-34397/tests-Add-a-test-for-signal-filtering-by-well-known-name.patch
CVE-2024-34397/tests-Ensure-that-unsubscribing-with-GetNameOwner-in-flig.patch
CVE-2024-34397/gdbusconnection-Allow-name-owners-to-have-the-syntax-of-a.patch
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment