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>
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.
Fixes: 145fb99b (untitled refactoring commit, 2006-12-12)
Helps: https://gitlab.freedesktop.org/dbus/dbus/-/issues/343
Signed-off-by: Simon McVittie <smcv@collabora.com>
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
(cherry picked from commit 63522f2887)
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).
(cherry picked from commit f8a7bffd5e)
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.
Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit 986611ad0f)
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.
Co-authored-by: Simon McVittie <smcv@collabora.com>
Resolves: dbus/dbus#457
(cherry picked from commit b159849e03)
This is helpful while debugging test failures.
Helps: dbus/dbus#457
Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit 8ee5d3e044)
It appears some freedesktop CI runners still have /builds/dbus/dbus
owned by a different user ID.
This reverts commit 3f6400b8b9.
Signed-off-by: Simon McVittie <smcv@collabora.com>
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__`.
Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
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__`.
Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
Previously, `retstr` would not be freed when `_dbus_string_append_len()`
or `_dbus_string_steal_data()` failed.
Fix those by:
* jumping to `_dbus_string_free()` when `_dbus_string_append_len()` fails
* ignoring the return value of `_dbus_string_free()`.
The latter works because in case of failure, `ret` will be set
to NULL by `_dbus_string_steal_data()`.
Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
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>
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>
The original code of 'ProgressNotification' had a wrong closing tag,
which should use '</signal>' instead of '</method>'
Signed-off-by: Yen-Chin, Lee <coldnew.tw@gmail.com>
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.
[smcv: expanded commit message]
Thanks: Evgeny Vereshchagin
Resolves: https://gitlab.freedesktop.org/dbus/dbus/-/issues/421
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
(cherry picked from commit 9950464761)
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>
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.
Signed-off-by: Jeremi Piotrowski <jpiotrowski@microsoft.com>
(cherry picked from commit 52b73d511b)
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.
Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit 25e1512c17)
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).
Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit dfa7eede44)
This cleans up checkouts that were subjected to `chown -R` prior to this.
Resolves: dbus/dbus#447
Signed-off-by: Simon McVittie <smcv@collabora.com>
(cherry picked from commit ce6af6721f)
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.
(cherry picked from commit 88dd9d5cc7)
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+
[1] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
[2] https://reviews.llvm.org/D133574
Signed-off-by: Khem Raj <raj.khem@gmail.com>
(cherry picked from commit c3e2c873ff)
