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.
Fixes: 683b14b9 "gdbus: Track name owners for signal subscriptions"
Resolves: 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>
(cherry picked from commit 7d65f6c5a2)
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>
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>
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>
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>
We will use this in a subsequent commit to prevent signals from an
impostor from being delivered to a subscriber.
To avoid message reordering leading to misleading situations, this does
not use the existing mechanism for watching bus name ownership, which
delivers the ownership changes to other main-contexts. Instead, it all
happens on the single thread used by the GDBusWorker, so the order in
which messages are received is the order in which they are processed.
[Backport to 2.66.x: fix minor conflicts]
Signed-off-by: Simon McVittie <smcv@collabora.com>
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>
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>
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>
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>
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>
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>
This somewhat duplicates test_connection_signals(), but is easier to
extend to cover different scenarios.
Each scenario is tested three times: once with lower-level
GDBusConnection APIs, once with the higher-level GDBusProxy (which
cannot implement all of the subscription scenarios, so some message
counts are lower), and once with both (to check that delivery of the
same message to multiple destinations is handled appropriately).
[Backport to 2.66.x: fix minor conflicts]
Signed-off-by: Simon McVittie <smcv@collabora.com>
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>
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 10e9a917be)
The `G_FILE_CREATE_REPLACE_DESTINATION` flag is equivalent to unlinking
the destination file and re-creating it from scratch. That did
previously work, but in the process the code would call `open(O_CREAT)`
on the file. If the file was a dangling symlink, this would create the
destination file (empty). That’s not an intended side-effect, and has
security implications if the symlink is controlled by a lower-privileged
process.
Fix that by not opening the destination file if it’s a symlink, and
adjusting the rest of the code to cope with
- the fact that `fd == -1` is not an error iff `is_symlink` is true,
- and that `original_stat` will contain the `lstat()` results for the
symlink now, rather than the `stat()` results for its target (again,
iff `is_symlink` is true).
This means that the target of the dangling symlink is no longer created,
which was the bug. The symlink itself continues to be replaced (as
before) with the new file — this is the intended behaviour of
`g_file_replace()`.
The behaviour for non-symlink cases, or cases where the symlink was not
dangling, should be unchanged.
Includes a unit test.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #2325
Since a following commit is going to add a new test which references
Gitlab, so it’s best to move the URI bases inside the test cases.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
The GDBusConnectionFlags and GDBusServerFlags can affect how we carry
out authentication and authorization, either making it more or less
restrictive, so it's desirable to "fail closed" if a program is compiled
against a new version of GLib but run against an old version.
Signed-off-by: Simon McVittie <smcv@collabora.com>
GByteArray uses guint for storing the length of the byte array, but it
also has a constructor (g_byte_array_new_take) that takes length as a
gsize. gsize may be larger than guint (64 bits for gsize vs 32 bits
for guint). It is possible to call the function with a value greater
than G_MAXUINT, which will result in silent length truncation. This
may happen as a result of unreffing GBytes into GByteArray, so rather
be loud about it.
(Test case tweaked by Philip Withnall.)
(Backport 2.66: Add #include gstrfuncsprivate.h in the test case for
`g_memdup2()`.)
These should never have been allowed; they will result in precondition
failures from the `GKeyFile` later on in the code.
A test will be added for this shortly.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fix an effective regression in commit
7781a9cbd2, which happens when
`convert_path()` is called with a `key` which contains no slashes. In
that case, the `key` is entirely the `basename`.
Prior to commit 7781a9cb, the code worked through a fluke of `i == -1`
cancelling out with the various additions in the `g_memdup()` call, and
effectively resulting in `g_strdup (key)`.
Spotted by Guido Berhoerster.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
The intention here was to assert that the length of the password fits
in a gssize. Passwords more than half the size of virtual memory are
probably excessive.
Fixes: a8b204ff "gtlspassword: Forbid very long TLS passwords"
Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit 61bb52ec42)
The public API `GIOChannel.line_term_len` is only a `guint`. Ensure that
nul-terminated strings passed to `g_io_channel_set_line_term()` can’t
exceed that length. Use `g_memdup2()` to avoid a warning (`g_memdup()`
is due to be deprecated), but not to avoid a bug, since it’s also
limited to `G_MAXUINT`.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2319
The public API `g_tls_password_set_value_full()` (and the vfunc it
invokes) can only accept a `gssize` length. Ensure that nul-terminated
strings passed to `g_tls_password_set_value()` can’t exceed that length.
Use `g_memdup2()` to avoid an overflow if they’re longer than
`G_MAXUINT` similarly.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2319
Don’t use an `int`, that’s potentially too small. In practical terms,
this is not a problem, since no socket address is going to be that big.
By making these changes we can use `g_memdup2()` without warnings,
though. Fewer warnings is good.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2319
Previously, the code in `convert_path()` could not handle keys longer
than `G_MAXINT`, and would overflow if that was exceeded.
Convert the code to use `gsize` and `g_memdup2()` throughout, and
change from identifying the position of the final slash in the string
using a signed offset `i`, to using a pointer to the character (and
`strrchr()`). This allows the slash to be at any position in a
`G_MAXSIZE`-long string, without sacrificing a bit of the offset for
indicating whether a slash was found.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2319
This allows it to handle strings up to length `G_MAXSIZE` — previously
it would overflow with such strings.
Update the several copies of it identically.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2319
