Simon McVittie [Mon, 18 Nov 2024 18:53:23 +0000 (18:53 +0000)]
bus: Don't create /run/dbus/containers
The `Containers1` interface is not stable or production-ready, and on
the 1.14.x branch it has always been compile-time-optional behind a build
option that cannot actually be enabled (it intentionally provokes a
compiler `#error`).
As discussed in <https://github.com/systemd/mkosi/issues/3189>, if we
use the `@DBUS_USER@` here, it makes `tmpfiles.d/dbus.conf` dependent
on having created the `@DBUS_USER@` via `sysusers.d` or some
appropriate distro-specific mechanism. This is problematic in distros
that split up the functionality of dbus into several layers, such as
Debian: the `@DBUS_USER@` conceptually belongs to the same layer as
the well-known system bus, but `/etc/machine-id` and
`/var/lib/dbus/machine-id` are also used by the well-known session bus,
which is orthogonal to the system bus; so we want `dbus.conf` to be in a
lower layer than the `@DBUS_USER@`.
Because the 1.14.x branch is never going to gain a working implementation
of the `Containers1` interface (that sort of feature change is
out-of-scope for a stable branch), we can simplify dbus.conf by never
creating /run/dbus/containers.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Simon McVittie [Sun, 27 Oct 2024 18:19:08 +0000 (18:19 +0000)]
tests: Only check whether the first 1024 fds are close-on-exec
On recent Linux systems, systemd sets the hard limit on the number of
file descriptors extremely high (about 1e9, compared with about 1e6 in
previous systemd versions or 4096 in the kernel's historical defaults),
and dbus raises its soft limit to match the hard limit. The result of
sysconf(_SC_OPEN_MAX) is based on the fd limit, and iterating linearly
through that many fds takes long enough for activation to time out.
This particular piece of code is just test instrumentation, which aims to
log (possibly fatal) warnings if any file descriptor is not close-on-exec
as it should be. In practice the test suite doesn't use anywhere near
a thousand fds, so it's sufficient to run this check against a much
smaller number of fds.
Resolves: https://gitlab.freedesktop.org/dbus/dbus/-/issues/527
Bug-Debian: https://bugs.debian.org/1086148 Signed-off-by: Simon McVittie <smcv@collabora.com>
Simon McVittie [Mon, 28 Oct 2024 16:57:21 +0000 (16:57 +0000)]
CI: Upgrade from Debian 11 'bullseye' to Debian 12 'bookworm'
Debian 11 is EOL for normal security support, and doesn't ship dbus
1.14.x anyway. Do our acceptance testing in Debian 12, which is not EOL,
and does include dbus 1.14.x.
Drop the buster build altogether: it is EOL, even for Debian LTS.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Simon McVittie [Fri, 1 Dec 2023 19:10:50 +0000 (19:10 +0000)]
CI: Set language environment variables to safe values
Recent versions of glibc support C.UTF-8 natively, and both Debian and
openSUSE (which we use for our CI) have been patching it into our glibc
versions for several years before that.
Helps: https://gitlab.freedesktop.org/dbus/dbus/-/issues/488 Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit 06606bd8cdf47b7adfb560bfd5c08689aec47bfd)
Simon McVittie [Tue, 1 Oct 2024 11:56:00 +0000 (12:56 +0100)]
CI: Stop working around httpredir.debian.org
All Debian Docker images from the last few years use deb.debian.org,
which is reliable, by default.
Debian 12 Docker images no longer have /etc/apt/sources.list (they use
/etc/apt/sources.list.d instead) so this is a prerequisite for moving
our CI to a supported version of Debian.
GCC 10.2.0 complains on opensuse mingw builds, ex:
disable-crash-handling.c: error: STABS debugging information is obsolete and not supported anymore [-Werror]
According to
https://gitlab.freedesktop.org/dbus/dbus/-/issues/133#note_129599,
-gdwarf-2 is the second best option.
[The gcc 12 in Debian 12 also no longer supports STABS, so this is a
prerequisite for being able to move our CI onto a supported version of
Debian. -smcv]
Simon McVittie [Mon, 28 Oct 2024 16:32:58 +0000 (16:32 +0000)]
CI: Install gettext-runtime for MSYS2 libintl
Our machinery for installing dependencies from MSYS2 only takes the
last matching line, but matches package names by substring. Now that
gettext-runtime and gettext-tools exist as separate packages, it was
installing that instead of the correct gettext-runtime, causing build
or test failures in various configurations that build against GLib.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Alan Coopersmith [Sun, 13 Oct 2024 19:33:06 +0000 (12:33 -0700)]
test: Move fd limit check into test_flood
Other tests have different requirements for fds (and already check for
them), so move this check into the specific test (mirroring what those
other tests already do) instead of blocking the whole test program,
incorrectly using autoconf test return codes instead of TAP protocol,
as discussed in #176.
Alan Coopersmith [Sun, 13 Oct 2024 19:24:09 +0000 (12:24 -0700)]
test: raise fd limit to maximum before running fdpass tests
Many of these tests require more than the default fd limit on some systems,
so raise the limit, if we can, to give them a better chance of being able
to run.
Simon McVittie [Fri, 15 Jul 2022 14:41:14 +0000 (15:41 +0100)]
test/bus: Break up dispatch test into three separate tests
This is really three separate test-cases: one for traditional
activation as a direct child process of the dbus-daemon, and two for
traditional activation (successful and failing) via the setuid
dbus-daemon-launch-helper on Unix.
The ones where activation succeeds are extremely slow, as a result of the
instrumentation for simulating malloc() failures combined with a large
number of memory operations, particularly when using AddressSanitizer.
Splitting up "OOM" tests like these has a disproportionately good impact
on the time they take, because the simulated malloc() failure
instrumentation repeats the entire test making the first malloc() fail,
then making the second malloc() fail, and so on. For allocation failures
in the second half of the test, this means we repeat the first half of
the test with no malloc() failures a very large number of times, which
is not a good use of time, because we already tested it successfully.
Even when not using the "OOM" instrumentation, splitting up these tests
lets them run in parallel, which is also a major time saving.
Needless to say, this speeds up testing considerably. On my modern
but unexceptional x86 laptop, in a typical debug build with Meson on
the 1.15.x branch, the old dispatch test took just over 21 minutes,
which drops to about 40 seconds each for the new normal-activation and
helper-activation tests (and for most of that time, they're running
in parallel, so the wall-clock time taken for the whole test suite is
somewhere around a minute).
In a debug build with Meson, gcc and AddressSanitizer on the 1.15.x
branch, the old dispatch test takes longer than my patience will allow,
and the new separate tests take about 5-6 minutes each. Reduce their
timeout accordingly, but not as far as the default for slow tests (5
minutes) to allow some headroom for AddressSanitizer or slower systems.
A side benefit of this is that it makes our testing more realistic. Each
call to bus_dispatch_test_conf() creates a new BusContext, which has the
side-effect of saving the initial file descriptor rlimit and expanding the
soft limit to match the hard limit. When we call bus_dispatch_test_conf()
more than once per test process, the second and subsequent calls will save
the increased rlimit as though it had been the original, and restore it
before exec'ing subprocesses, resulting in a potentially extremely high
soft limit which means our debug instrumentation takes a very long time
to iterate through all possible fds (see dbus#527).
Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit bef88fd5627f2d990915c9009982107a8e329ef5)
Mitigates: https://gitlab.freedesktop.org/dbus/dbus/-/issues/527
configure: Fix check for Solaris audit API to work with gcc 14
Need to declare a variable type to avoid gcc 14 error:
configure:22502: checking for ADT API
conftest.c:107:1: warning: data definition has no type or storage class
107 | adt_user_context = ADT_USER;
| ^~~~~~~~~~~~~~~~
conftest.c:107:1: error: type defaults to 'int' in declaration of
'adt_user_context' [-Wimplicit-int]
Fixes: ab1eb1fd ("Bug 15740: Solaris/ADT auditing support (simon zheng)") Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Simon McVittie [Thu, 29 Jun 2023 18:52:39 +0000 (19:52 +0100)]
bus: When failing to reload client policy, continue iteration
If we have a large number of connections to the bus, and we fail to
reload the policy for one of them (perhaps because its uid no longer
exists in the system user database), previously we would crash, which
is obviously unintended. After the previous commit, we would stop
iteration through the list of client connections, which doesn't seem
great either: one bad connection shouldn't prevent us from reloading
the rest of our state.
Instead, let's distinguish between new connections (where we want
failure to establish a security policy to be fatal), and pre-existing
connections (where the current security policy is presumably good
enough to keep using if we have nothing better). If we're unable to
reload the policy for a pre-existing connection, log a warning and
carry on iterating.
Helps: https://gitlab.freedesktop.org/dbus/dbus/-/issues/343 Signed-off-by: Simon McVittie <smcv@collabora.com>
Simon McVittie [Thu, 29 Jun 2023 15:06:39 +0000 (16:06 +0100)]
userdb: Add proper error reporting when getting groups from a uid
Previously, if dbus_connection_get_unix_user() succeeded but
_dbus_unix_groups_from_uid() failed, then bus_connection_get_unix_groups()
would incorrectly fail without setting the error indicator, resulting
in "(null)" being logged, which is rather unhelpful.
This also lets us distinguish between ENOMEM and other errors, such as
the uid not existing in the system's user database.
Peter Benie [Fri, 23 Jun 2023 10:51:00 +0000 (11:51 +0100)]
bus: Don't crash if bus_context_create_client_policy() fails
If policy creation fails, we can't usefully leave a NULL policy in the
BusConnectionData. If we did, the next attempt to reload policy would
crash with a NULL dereference when we tried to unref it, or with
an assertion failure.
One situation in which we can legitimately fail to create a client policy
is an out-of-memory condition. Another is if we are unable to look up a
connection's supplementary groups with SO_PEERGROUPS, and also unable to
look up the connection's uid's groups in the system user database, for
example because it belongs to a user account that has been deleted (which
is sysadmin error, but can happen, particularly in automated test systems)
or because a service required by a Name Service Switch plugin has failed.
Keeping the last known policy is consistent with what happens to all
the connections that are after this one in iteration order: after we
early-return, all of those connections retain their previous policies
(which doesn't seem ideal either, but that's how this has always worked).
[smcv: Add commit message] Co-authored-by: Simon McVittie <smcv@collabora.com>
Resolves: https://gitlab.freedesktop.org/dbus/dbus/-/issues/343
dbus-sysdeps-unix: Handle empty supplementary group list
It is valid for getsockopt(SO_PEERGROUPS) to return len=0; that
indicates the process has no supplementary groups. Rather than failing,
simply use the returned empty list (and add the primary GID to it).
Without this change, calling GetConnectionCredentials on a bus name
owned by a process with no supplementary groups does not return the
UnixGroupIDs field, even though it is easy to determine that the process
only has one GID (the primary GID).
Simon McVittie [Mon, 5 Jun 2023 17:51:22 +0000 (18:51 +0100)]
monitor test: Reproduce dbus/dbus#457
The exact failure mode reported in dbus/dbus#457 is quite difficult
to achieve in a reliable way in a unit test, because we'd have to send
enough messages to a client to fill up its queue, then stop that client
from draining its queue, while still triggering a message that gets a
reply from the bus driver. However, we can trigger the same crash in a
slightly different way by not allowing the client to receive a
particular message. I chose NameAcquired.
hongjinghao [Mon, 5 Jun 2023 17:17:06 +0000 (18:17 +0100)]
bus: Assign a serial number for messages from the driver
Normally, it's enough to rely on a message being given a serial number
by the DBusConnection just before it is actually sent. However, in the
rare case where the policy blocks the driver from sending a message
(due to a deny rule or the outgoing message quota being full), we need
to get a valid serial number sooner, so that we can copy it into the
DBUS_HEADER_FIELD_REPLY_SERIAL field (which is mandatory) in the error
message sent to monitors. Otherwise, the dbus-daemon will crash with
an assertion failure if at least one Monitoring client is attached,
because zero is not a valid serial number to copy.
This fixes a denial-of-service vulnerability: if a privileged user is
monitoring the well-known system bus using a Monitoring client like
dbus-monitor or `busctl monitor`, then an unprivileged user can cause
denial-of-service by triggering this crash. A mitigation for this
vulnerability is to avoid attaching Monitoring clients to the system
bus when they are not needed. If there are no Monitoring clients, then
the vulnerable code is not reached.
Barnabás Pőcze [Thu, 4 May 2023 14:37:57 +0000 (16:37 +0200)]
dbus-spawn-win: use `_DBUS_FUNCTION_NAME` instead of `__FUNCTION__`
dbus-internals.h already defines a macro which expands to the name
of the current function based on C standard version, etc. So use
that instead of hard-coding `__FUNCTION__`.
Barnabás Pőcze [Thu, 4 May 2023 14:26:31 +0000 (16:26 +0200)]
dbus-internals: use `_DBUS_FUNCTION_NAME` in `_dbus_verbose()`
dbus-internals.h already defines a macro which expands to the name
of the current function based on C standard version, etc. So use
that instead of hard-coding `__FUNCTION__`.
Simon McVittie [Fri, 12 May 2023 14:23:27 +0000 (15:23 +0100)]
CI: Only run for pushes to dbus
In practice the pipeline is going to fail for namespaces other than
dbus, so don't waste time on trying to run it there; only run the
detached pipeline for the MR.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Simon McVittie [Fri, 12 May 2023 14:00:34 +0000 (15:00 +0100)]
CI: Run a detached pipeline for merge requests
After abuses of fdo infrastructure were mitigated in
freedesktop/freedesktop#540, contributors cannot usually run pipelines
in their own forks of dbus.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Ralf Habacker [Thu, 19 Jan 2023 15:17:43 +0000 (16:17 +0100)]
_dbus_string_skip_blank(): Let the final assert follow the previous search
This appears to have been a copy/paste mistake. If only blanks (defined as
spaces or tabs) were removed, then it cannot be right to check for white
space (defined as spaces, tabs, carriage return or linefeed) afterwards.
If libdbus was compiled with assertions enabled, then this is a
denial-of-service issue for dbus-daemon or other users of DBusServer:
an unauthenticated user with access to the server's socket can send
whitespace that triggers this assertion failure. We recommend that
production versions of dbus, for example in OS distributions, should be
compiled with checks but without assertions.
Dave Jones [Tue, 3 Jan 2023 18:42:51 +0000 (18:42 +0000)]
Permit access to /tmp dbus socket
After 6e48c317 the test-apparmor-activation test fails as it can no
longer access the dbus socket in /tmp. This commit updates the apparmor
profile used within the test
Simon McVittie [Tue, 11 Oct 2022 20:08:05 +0000 (21:08 +0100)]
m4: Stop shipping our own copy of pkg.m4
The canonical way to find these macro is to run autogen.sh or autoreconf
while pkg-config (or pkgconf) is installed, which will copy an up-to-date
version of the macros from /usr/share/aclocal. The `make dist` archive
will include a bundled copy of those macros, but the git repository
doesn't need them.
Signed-off-by: Simon McVittie <smcv@collabora.com>
bus/selinux: Move vsnprintf call to avoid va_list reuse
In log_callback() the same va_list is reused for a call to vsnprintf and
vsyslog. A va_list can't be reused in this manner, such use is undefined
behavior that changes depending on glibc version.
In current glibc versions a segfault can be observed from the callsite at
bus/selinux.c:412. When trying to log a non-auditable event, the segfault
happens in strlen inside vsyslog.
Moving the call to vsnprintf closer to audit_log_user_avc_message (which is
followed by a 'goto out') avoids the reuse and segfault.
Simon McVittie [Mon, 6 Feb 2023 13:24:11 +0000 (13:24 +0000)]
CI: Re-run some tests as root or as non-root, as appropriate
On Gitlab-CI we're always running the overall script as root (and
therefore we'll only enter the code path to re-run as non-root),
but when using these scripts for manual testing they might be run as
non-root to begin with.
Simon McVittie [Mon, 6 Feb 2023 13:23:05 +0000 (13:23 +0000)]
CI: Remove vestigial support for re-running tests in a Docker container
Travis CI needed this, but Gitlab-CI always runs our tests in a Docker
container of our choice, so there's never any need to enter another
(and it's not allowed anyway).
Simon McVittie [Mon, 6 Feb 2023 13:13:04 +0000 (13:13 +0000)]
CI: Don't change ownership of source directory
These CI scripts were originally used on Travis-CI, which starts all
builds as an ordinary user that has the ability to become root via `sudo`.
On Gitlab-CI, we don't need that: we start as uid 0, and can do the
whole CI run like that. This also means we get somewhat better test
coverage, because some of our unit tests benefit from being run as uid 0.
The only test coverage we lose by being uid 0 is that
test_pending_fd_timeout() in test/dbus-daemon.c is skipped, because
uid 0 bypasses the limit that's under test there.
Khem Raj [Sat, 14 Jan 2023 20:39:43 +0000 (12:39 -0800)]
Define _DBUS_ALIGNOF using _Alignof when using C11 or newer
WG14 N2350 made very clear that it is an UB having type definitions
within "offsetof" [1]. This patch changes the implementation of macro
_DBUS_ALIGNOF to builtin "_Alignof" to avoid undefined behavior.
clang 16+ has started to diagnose this [2]
Fixes build when using -std >= gnu11 and using clang16+
Ralf Habacker [Tue, 3 Jan 2023 13:29:14 +0000 (14:29 +0100)]
Fix data race in multithreaded application
This commit fixes a data race condition discovered by the
gcc thread sanitizer by also locking the associated mutex
when reading the corresponding counter.
config-parser: no longer get past the last NULL passed to locate_attributes
Fixes: bc86794f23fa53
Fixes:
```
==302818==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7ffd6ac253c0 at pc 0x000000534d0b bp 0x7ffd6ac24e10 sp 0x7ffd6ac24e08
READ of size 8 at 0x7ffd6ac253c0 thread T0
#0 0x534d0a in locate_attributes /home/vagrant/dbus/build/../bus/config-parser.c:658:16
#1 0x52ea3f in start_busconfig_child /home/vagrant/dbus/build/../bus/config-parser.c:1080:12
#2 0x52cca4 in bus_config_parser_start_element /home/vagrant/dbus/build/../bus/config-parser.c:2039:14
#3 0x52b82b in expat_StartElementHandler /home/vagrant/dbus/build/../bus/config-loader-expat.c:107:8
#4 0x7f2179f2d2bd (/lib64/libexpat.so.1+0xd2bd) (BuildId: 0165eed77c910f6ef2227d21afa9c5c5ed5849c2)
#5 0x7f2179f2aed3 (/lib64/libexpat.so.1+0xaed3) (BuildId: 0165eed77c910f6ef2227d21afa9c5c5ed5849c2)
#6 0x7f2179f2c9ec (/lib64/libexpat.so.1+0xc9ec) (BuildId: 0165eed77c910f6ef2227d21afa9c5c5ed5849c2)
#7 0x7f2179f30a8e in XML_ParseBuffer (/lib64/libexpat.so.1+0x10a8e) (BuildId: 0165eed77c910f6ef2227d21afa9c5c5ed5849c2)
#8 0x52b040 in bus_config_load /home/vagrant/dbus/build/../bus/config-loader-expat.c:259:9
#9 0x523c8a in bus_context_new /home/vagrant/dbus/build/../bus/bus.c:828:12
#10 0x521056 in main /home/vagrant/dbus/build/../bus/main.c:716:13
#11 0x7f2179a2954f in __libc_start_call_main (/lib64/libc.so.6+0x2954f) (BuildId: 9c5863396a11aab52ae8918ae01a362cefa855fe)
#12 0x7f2179a29608 in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x29608) (BuildId: 9c5863396a11aab52ae8918ae01a362cefa855fe)
#13 0x42a914 in _start (/home/vagrant/dbus/build/bus/dbus-daemon+0x42a914) (BuildId: df5369f85137975aff9bd398ae859706cc3c52ff)
Address 0x7ffd6ac253c0 is located in stack of thread T0 at offset 0 in frame
#0 0x52cfaf in start_busconfig_child /home/vagrant/dbus/build/../bus/config-parser.c:733
```
Simon McVittie [Fri, 30 Sep 2022 13:01:05 +0000 (14:01 +0100)]
test: Parse a message with a byteswapped Unix fd index
Reproduces: https://gitlab.freedesktop.org/dbus/dbus/-/issues/417 Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit bef693f442d854505e7013fd31efe41747d7493c)
[backport to 1.14.x: discard Meson build system updates]
Simon McVittie [Fri, 30 Sep 2022 13:00:08 +0000 (14:00 +0100)]
test: Add infrastructure to parse valid raw message blobs
Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit 7a2c13d21be702c7b5b7288fb82a60adc5bd7378)
[backport to 1.14.x: discard Meson build system updates]
Simon McVittie [Fri, 30 Sep 2022 12:46:31 +0000 (13:46 +0100)]
dbus-marshal-byteswap: Byte-swap Unix fd indexes if needed
When a D-Bus message includes attached file descriptors, the body of the
message contains unsigned 32-bit indexes pointing into an out-of-band
array of file descriptors. Some D-Bus APIs like GLib's GDBus refer to
these indexes as "handles" for the associated fds (not to be confused
with a Windows HANDLE, which is a kernel object).
The assertion message removed by this commit is arguably correct up to
a point: fd-passing is only reasonable on a local machine, and no known
operating system allows processes of differing endianness even on a
multi-endian ARM or PowerPC CPU, so it makes little sense for the sender
to specify a byte-order that differs from the byte-order of the recipient.
However, this doesn't account for the fact that a malicious sender
doesn't have to restrict itself to only doing things that make sense.
On a system with untrusted local users, a message sender could crash
the system dbus-daemon (a denial of service) by sending a message in
the opposite endianness that contains handles to file descriptors.
Before this commit, if assertions are enabled, attempting to byteswap
a fd index would cleanly crash the message recipient with an assertion
failure. If assertions are disabled, attempting to byteswap a fd index
would silently do nothing without advancing the pointer p, causing the
message's type and the pointer into its contents to go out of sync, which
can result in a subsequent crash (the crash demonstrated by fuzzing was
a use-after-free, but other failure modes might be possible).
In principle we could resolve this by rejecting wrong-endianness messages
from a local sender, but it's actually simpler and less code to treat
wrong-endianness messages as valid and byteswap them.
Thanks: Evgeny Vereshchagin Fixes: ba7daa60 "unix-fd: add basic marshalling code for unix fds"
Resolves: https://gitlab.freedesktop.org/dbus/dbus/-/issues/417
Resolves: CVE-2022-42012 Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit 236f16e444e88a984cf12b09225e0f8efa6c5b44)
Simon McVittie [Mon, 12 Sep 2022 12:28:47 +0000 (13:28 +0100)]
test: Add targeted tests for dbus/dbus#413, dbus/dbus#418
Unlike the message-internals test, these do not rely on extra debug
instrumentation in libdbus, and so can be used for "as-installed"
testing. (However, they do require GLib.)
Reproduces: https://gitlab.freedesktop.org/dbus/dbus/-/issues/413
Reproduces: https://gitlab.freedesktop.org/dbus/dbus/-/issues/418 Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit 3ef342410a1cefe3d0bfaf46279c6517f4b44a26)
Simon McVittie [Mon, 12 Sep 2022 12:24:28 +0000 (13:24 +0100)]
test: Export G_TEST_BUILDDIR, G_TEST_SRCDIR
These environment variables are used by GLib's g_test_build_filename()
and related convenience functions, which make it easier for unit tests
to find data files in a way that works for both build-time tests and
"as-installed" tests. During "as-installed" testing, both variables
will normally be unset, and GLib uses the directory containing the
executable. In most cases that results in the right thing happening, and
this will also be true for dbus, since we install the test executables
in ${libexecdir}/installed-tests, helper executables in the same place,
and test data in ${libexecdir}/installed-tests/data.
Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit 37e01259435a28b11a087867ea199cd09f0a5cff)
[backport to 1.14.x: discard Meson build system updates]
Simon McVittie [Mon, 12 Sep 2022 12:28:29 +0000 (13:28 +0100)]
test/data: Add messages that reproduce dbus/dbus#413, dbus/dbus#418
In debug builds with "embedded tests" enabled, these will automatically
be used as input for the message-internals test.
Some of the messages themselves are output from a fuzzer, others are
simplifications to include only one reason for lack of validity per
message.
I've included an annotated hex-dump for each message here, but the dbus
test suite doesn't currently know how to convert hex to binary, so I've
also committed the corresponding binary. See the comment at the top of
each hex-dump for how to create the binary version (which requires the
xxd tool shipped with vim).
It would be nice for the dbus test suite to be able to convert the
annotated hex-dump to binary, either at build-time with a Python script
or at runtime by loading the text file and decoding the hex, but I don't
want to block on that for dbus/dbus#413 and dbus/dbus#418.
Reproduces: https://gitlab.freedesktop.org/dbus/dbus/-/issues/413
Reproduces: https://gitlab.freedesktop.org/dbus/dbus/-/issues/418 Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit d633016f7340f83142f19f4bcac08c57e1f2fd2f)
[backport to 1.14.x: discard Meson build system updates]
Simon McVittie [Mon, 12 Sep 2022 12:14:18 +0000 (13:14 +0100)]
dbus-marshal-validate: Validate length of arrays of fixed-length items
This fast-path previously did not check that the array was made up
of an integer number of items. This could lead to assertion failures
and out-of-bounds accesses during subsequent message processing (which
assumes that the message has already been validated), particularly after
the addition of _dbus_header_remove_unknown_fields(), which makes it
more likely that dbus-daemon will apply non-trivial edits to messages.
Thanks: Evgeny Vereshchagin Fixes: e61f13cf "Bug 18064 - more efficient validation for fixed-size type arrays"
Resolves: https://gitlab.freedesktop.org/dbus/dbus/-/issues/413
Resolves: CVE-2022-42011 Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit 079bbf16186e87fb0157adf8951f19864bc2ed69)
Simon McVittie [Tue, 13 Sep 2022 14:10:22 +0000 (15:10 +0100)]
dbus-marshal-validate: Check brackets in signature nest correctly
In debug builds with assertions enabled, a signature with incorrectly
nested `()` and `{}`, for example `a{i(u}` or `(a{ii)}`, could result
in an assertion failure.
In production builds without assertions enabled, a signature with
incorrectly nested `()` and `{}` could potentially result in a crash
or incorrect message parsing, although we do not have a concrete example
of either of these failure modes.
Thanks: Evgeny Vereshchagin
Resolves: https://gitlab.freedesktop.org/dbus/dbus/-/issues/418
Resolves: CVE-2022-42010 Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit 9d07424e9011e3bbe535e83043d335f3093d2916)
Simon McVittie [Thu, 29 Sep 2022 15:50:07 +0000 (16:50 +0100)]
dbus-server-socket: Make unix:tmpdir equivalent to unix:dir
On Linux, there are two classes of AF_UNIX socket, which D-Bus refers
to as unix:path=... (portable to non-Linux systems) and unix:abstract=...
(not portable).
Back in 2003 when dbus gained support for abstract Unix sockets on Linux,
everyone thought they were better in every way than path-based Unix
sockets: if a DBusServer crashes or is terminated abnormally, there's
no detritus left in the filesystem. What's not to like? As a result,
since commit a70b042f (2003-06-04), when a DBusServer listens on a
unix:tmpdir=... address on Linux, the default is for the result to be
a unix:abstract=... address, with unix:path=... addresses only used on
non-Linux platforms.
However, the world has changed in the last 19 years, and namespace-based
Linux containers (which didn't exist in 2003) are now very popular. This
makes abstract sockets problematic.
Abstract sockets are tied to the network namespace, which is
all-or-nothing: if a container is to access the Internet without using
some sort of proxy or intermediary (like slirp4netns) then it needs to
share the network namespace with the host system, and that implies
sharing all abstract sockets with the host system. If the well-known
session bus is listening on an abstract socket, then it's a sandbox
escape route for any sandboxed or containerized app running under the
same uid. Conversely, if a container is *not* sharing the network
namespace with the host system, then it cannot access a session bus that
is listening on an abstract socket without using some sort of proxy
(like xdg-dbus-proxy), even if it isn't intended to impose a security
boundary and giving it direct access to the session bus would have been
more desirable.
Path-based sockets do not have this problem because they exist in the
filesystem (part of the "everything is a file" Unix philosophy),
allowing mount namespaces and bind-mounts to be used to share or
unshare them selectively.
On systems with `systemd --user` where dbus has been configured with
`--enable-user-session`, in general the session bus will already be
using a path-based socket for the "user bus", disregarding the listening
address specified in /usr/share/dbus-1/session.conf. The default in many
recent Linux distributions is either to use dbus-daemon in this way, or
to use dbus-broker, a reimplementation of the message bus service which
has similar "user bus" behaviour.
However, the <listen> address in session.conf is used when dbus-launch(1)
or dbus-run-session(1) is used to start a session bus, either manually,
via autolaunching, or via system integration glue in operating systems
that are not using `systemd --user`. This will occur particularly often
in operating systems that boot using a non-systemd init system.
Making unix:tmpdir=/tmp equivalent to unix:dir=/tmp ensures that the
well-known session bus listens on a path-based socket, allowing container
and sandboxing frameworks to mediate access to it in the same way they
would for the user bus. The D-Bus Specification already allows (but does
not require) this behaviour, because it is the only thing that was
implementable on non-Linux systems such as *BSD.
This change has the potential to cause regressions. If a container
framework enters a chroot or unshares the mount namespace but does not
unshare the network namespace, and is relying on the ability for a
process inside a container to access the session bus outside the
container via its abstract socket, then that assumption will be broken
by this change. Some use cases of schroot(1) are likely to suffer from
this. However, container frameworks with that assumption would already
have found that it does not hold when using the user bus, and it is
necessary to break that assumption if we want it to be possible to apply
application-level sandboxing in a secure way.
Another potential regression from this change is that if a dbus-daemon
is terminated abnormally, it will leave a socket in /tmp. Distributors
of operating systems where heavy use of dbus-launch(1) is expected might
wish to run dbus-cleanup-sockets(1) periodically.
Resolves: https://gitlab.freedesktop.org/dbus/dbus/-/issues/416 Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit f01382ae310c7d63790c07ed280f575d91ea57b8)
[backport to 1.14.x: adjust to absence of d98c98d1 in this branch]
projects / thirdparty / dbus.git / log
