I reviewed dbus-broker 37-5ubuntu3 as checked into stonking. This shouldn't be
considered a full audit but rather a quick gauge of maintainability. This
review follows up on the partial review of v33-1 in comment #5; the package
has since gained the AppArmor mediation patches (now also in Debian, see
comment #15/#18) and the `dbus-run-session` blocker has been resolved by the
decision to retain `src:dbus` in main (comment #17/#25).
dbus-broker is an implementation of a D-Bus message bus as defined by the
D-Bus specification. It is a drop-in replacement for `dbus-daemon` (the
reference implementation) intended to provide higher performance and
reliability. It is written exclusively for Linux and makes use of modern
kernel features. The system bus runs as the unprivileged `messagebus` user;
the launcher (`dbus-broker-launch`) is started by systemd socket activation,
forks the broker, and drops privileges. It parses untrusted D-Bus wire
protocol from local peers via the embedded `libcdvar-1` type-driven
deserializer.
- CVE History
- $UCT/scripts/pkg_history / pkg_status: previous issues were
CVE-2022-31212 and CVE-2022-31213 (memory corruption from untrusted
D-Bus input, https://sec-consult.com/vulnerability-lab/advisory/...).
Both were fixed upstream in a timely fashion (resolved in Ubuntu series
> Kinetic); v37 is well past the fixes. Upstream responded quickly to
the report.
- Upstream now runs static analyzers and fuzzers via GitHub Actions and is
scanned by Coverity (https://scan.coverity.com/projects/dbus-broker).
- Upstream still does **not** have a formal Security Policy / coordinated
disclosure process. Recommend one is added (see non-blocking notes).
- Build-Depends
- debhelper-compat (= 13), dh-package-notes, libapparmor-dev (>= 3.0),
libaudit-dev, libcap-ng-dev, libdbus-1-dev, libexpat1-dev,
libselinux-dev, libsystemd-dev, meson (>= 0.60), pkgconf,
python3-docutils <!nodoc>, systemd-dev
(see dbus-broker_37-5ubuntu3.dsc).
- No encryption libraries. `libsystemd-dev` is needed only by the launcher
(`dbus-broker-launch`); the broker itself is not systemd-dependent.
- Runtime ELF deps: libapparmor.so.1, libaudit.so.1, libc.so.6,
libcap-ng.so.0, libexpat.so.1, libm.so.6, libselinux.so.1,
libsystemd.so.0 — all standard and already in main.
- pre/post inst/rm scripts
- `postinst`: on `triggered`, reloads `dbus-broker.service` if active; on
`configure`, recommends a reboot (writes `/run/reboot-required[.pkgs]`)
when replacing `dbus-daemon` or upgrading across versions; for upgrades
from << 37 writes an ephemeral systemd override to switch the unit type
to `notify` before reloading (removed-after:trixie). Standard
`dh_installsystemd`/`dh_installsystemduser` snippets enable the system
and user units.
- `postrm`: `dh_installsystemd` daemon-reload on remove, purge on purge;
on `remove` drops the `dbus.service` alias so other implementations can
take over.
- `prerm`: intentionally empty (overrides the `dh_installsystemd` stop-on-
remove snippet) so a running system/session is not killed on package
removal (matches the dbus-daemon behaviour, Debian #980541).
- No bashisms; shellcheck is clean.
- init scripts
- none (systemd-only, consistent with Ubuntu not supporting alternative
inits).
- systemd units
- `dbus-broker.service` (system): `Type=notify-reload`, `Sockets=dbus.socket`,
`OOMScoreAdjust=-900`, `LimitNOFILE=16384`, `ProtectSystem=full`,
`PrivateTmp=true`, `PrivateDevices=true`,
`ExecStart=.../dbus-broker-launch --scope system --audit`, aliases as
`dbus.service` (src/units/system/dbus-broker.service.in).
- `dbus-broker.service` (user): `Type=notify-reload`, `Sockets=dbus.socket`,
`Slice=session.slice`, `ExecStart=.../dbus-broker-launch --scope user`
(src/units/user/dbus-broker.service.in).
- Both are socket-activated via `dbus.socket` and do not bind paths
themselves.
- dbus services
- none shipped by this package.
- setuid binaries
- none. No FS capabilities (setcap/getcap) on the shipped binaries
(packaging.txt: `setuid()` / `setuid_fs()` / `fs capabilities` empty).
- binaries in PATH
- `/usr/bin/dbus-broker` (236032 B), `/usr/bin/dbus-broker-launch`
(129496 B), both root:root 0755 (packaging.txt).
- Both are fully hardened: PIE, stack protector, Fortify, RELRO,
immediate binding, CFI. `dbus-broker` has stack-clash protection;
`dbus-broker-launch` is missing `-fstack-clash-protection` (minor, see
non-blocking notes).
- sudo fragments
- none.
- polkit files
- none. Authorization is handled by the internal D-Bus policy engine
(`src/bus/policy.c`, `src/launch/policy.c`) plus optional
AppArmor/SELinux MAC.
- udev rules
- none.
- unit tests / autopkgtests
- Build-time test suite runs (meson `test`); the build log shows the
unit tests OK (a few `ERROR` lines are sandbox/test-environment
artifacts on `memfd`/error-injection, not source failures; `Fail: 0`).
- `Testsuite: autopkgtest` in the .dsc; autopkgtest runs the upstream
test suite and is currently passing on all release architectures.
- The dfuzzer-based D-Bus service fuzzing noted in comment #6 remains a
pre-existing confinement gap (neither dbus nor dbus-broker ship
AppArmor profiles by default) and does not affect the MIR outcome.
- cron jobs
- none.
- Build logs
- Clean. No compiler errors. The only compiler warnings are 15x benign
`-Waddress` in `subprojects/libcdvar-1/src/test-api.c:38-52` ("address
of `c_dvar_type_*` will always evaluate as true", test code). No
`-Wformat-security`, `-Wstringop-overflow`, `-Warray-bounds` or
similar. `lintian --pedantic` reports only
`newer-standards-version 4.7.4`, `orig-tarball-missing-upstream-
signature`, and a stray `.github`/`.gitmodules` in the source tree
(should be cleaned from the upstream orig tarball).
- Processes spawned
- The only production exec is `execve("/usr/bin/dbus-broker", argv,
environ)` in `launcher_run_child()` (src/launch/launcher.c:365). The
binary path is a compile-time constant (`BINDIR "/dbus-broker"`,
launcher.c:46); `argv` is a fixed array of flags plus numeric strings
produced by `snprintf` into stack buffers with bounds-checked return
values (launcher.c:350-363). No `popen`/`system`/`posix_spawn`
anywhere. No shell is involved, so no shell-injection surface.
- `fork()` is used in `launcher_fork()` (launcher.c:391); the child is
watched via `sd_event_add_child`.
- Memory management
- Defensive throughout. Allocations consistently use the
`sizeof(*ptr) + n` idiom with `n` derived from a validated length,
e.g. `src/broker/controller.c:52`, `src/bus/name.c:234`,
`src/launch/config.c:61`, `src/util/selinux.c:121`.
- `realloc` results are checked and assigned to a temporary before
overwriting the live pointer (`src/util/sockopt.c:44,105,177`,
`src/util/fs.c:102`, `subprojects/libcdvar-1/src/c-dvar-writer.c:37`).
- `strcpy`/`memcpy` are used only after the destination was allocated
with `strlen(src)+1` and the length pre-computed, e.g. the AppArmor
query-string builders in `src/util/apparmor.c:230-298`, match-rule
copies in `src/bus/match.c:475,531,589`, policy strings
`src/bus/policy.c:193`. `asprintf` return values are always checked.
- Heavy use overall (it is a message broker), cursory view looks OK.
- File IO
- All `open`/`fopen` use `O_CLOEXEC` / `"re"` and fixed or root-controlled
paths: service files `src/launch/launcher.c:538`, config includes
`src/launch/config.c:1305` (`O_RDONLY|O_CLOEXEC|O_NOCTTY`),
`/etc/passwd`+`/etc/group` via `nss-cache.c:251,273`, AppArmor
`/sys/module/apparmor/...` and `/sys/kernel/security/apparmor/...`
(`src/util/apparmor.c:45,87`), `/proc/self/...` (`src/util/proc.c`).
- No `umask` manipulation, no `O_CREAT` on attacker-controlled paths,
no `chmod` of attacker files (`fchmod` in `src/util/misc.c:109`
preserves the existing mode on an already-open fd).
- Config include recursion is guarded against cycles
(`src/launch/config.c:1292-1297`).
- Logging
- `src/util/log.c`. `log_vappendf` (log.c:601) uses `vsnprintf` into a
sized buffer with truncation checks; no `syslog(3)` direct calls
(messages go to journald or a syslog/stream socket).
- Every `*printf`-style log call uses a **compile-time constant format
string**. The one place a dynamic string reaches the logger (the
AppArmor audit-log fallback, patch 0021) is written defensively as
`log_commitf(ctx, "%s", message)` (`src/util/apparmor.c:202`), i.e. the
attacker-influenced message is an argument, not the format. No tainted
format-string risk.
- Environment variable usage
- Only `XDG_RUNTIME_DIR`, `XDG_DATA_HOME`, `XDG_DATA_DIRS` in the
launcher (`src/launch/launcher.c:863,887,918`), used solely as
directory prefixes for read-only `.service`-file lookup, with
`passwd->pw_dir`-based defaults (`launcher.c:895`). Nothing is used as
a command, a shell path, or a format string; values are not re-exported
to untrusted children. The launcher is started by systemd with a
sanitized environment.
- Use of privileged functions
- Two privilege-drop paths, both with the **correct** ordering
`setgroups(0,NULL)` → `setgid(gid)` → `setuid(uid)`:
- launcher parent: `launcher.c:1455` → `util_drop_permissions()`
(`src/util/misc.c:254-269`); `setgroups` failure is non-fatal for
dbus-daemon compatibility (launcher runs as `messagebus` with no
supplementary groups).
- broker child: `launcher.c:301` → `util_audit_drop_permissions()`
(`src/util/audit.c:33-91`); when `euid==0` uses
`capng_change_id(uid, gid, CAPNG_DROP_SUPP_GRP)` and then raises
only `CAP_AUDIT_WRITE` as ambient (`audit.c:79,84`).
- UID/GID are resolved once from `/etc/passwd`+`/etc/group` at config-
parse time and stored as `uint32`; not re-resolved before use → no
TOCTOU on credentials. The child drops privileges *before* opening the
journal socket and before `execve` (`launcher.c:300-304`).
- `ioctl(fd, SIOCOUTQ, &v)` in `log_stream_send()`
(`src/util/log.c:338`) and `socket_dispatch_write()`
(`src/dbus/socket.c:746`): read-only queries of the socket send-buffer
byte count, on internal fds, return value checked. Used only to decide
whether to drop a log message in lossy mode. Safe (still present and
correct, as in the 2023 review).
- Use of cryptography / random number sources etc
- No cryptographic code (no OpenSSL/GnuTLS; the `== encryption ==`
section of the code scan is empty). No TLS, no certificate handling.
- Randomness is sourced from `getauxval(AT_RANDOM)` (16 kernel-provided
bytes): bus GUID seeding (`src/bus/bus.c:41-43`) and the libexpat hash
salt (`src/launch/config.c:1254-1256`, with an explicit comment that
this avoids a hidden `/dev/urandom` dependency). These are identifiers,
not security tokens; dbus-daemon uses the same approach.
- Use of temp files
- No `mkstemp`/`mkstemps`/`tmpnam`/`tempnam`/`tmpfile`. The only `/tmp`
reference is in test code (`test/dbus/util-broker.c:279`,
`unix:tmpdir=/tmp` which is an abstract AF_UNIX socket, not a
predictable file).
- Use of networking
- All sockets are `AF_UNIX`; no `AF_INET`/`AF_INET6` in production code.
- The untrusted-input boundary is `recvmsg` in
`src/dbus/socket.c:545` with `MSG_DONTWAIT|MSG_CMSG_CLOEXEC` (received
fds are close-on-exec and counted/validated in `src/util/fdlist.c`).
- Incoming message headers are parsed in `src/dbus/message.c:51-91`
(`message_new_incoming`): endianness validated, sizes read in 64-bit,
`n_data` capped at `MESSAGE_SIZE_MAX` (128 MiB, `src/dbus/message.h:20`).
Header fields and body are decoded via the embedded `libcdvar-1`
reader which performs strict type/bounds checks and enforces
full-consumption (`c_dvar_end_read`, `c-dvar-reader.c:694-695`); array
reads verify `u32 <= n_buffer` (`c-dvar-reader.c:239-242`). Invalid
messages yield `MESSAGE_E_*` and the peer is disconnected
(`src/bus/peer.c:95,105`). The listener socket is pre-bound by systemd
and inherited via `sd_listen_fds`; the broker does not `bind` to
attacker-named paths.
- Use of WebKit
- none.
- Use of PolicyKit
- none
- Any significant cppcheck results
- 27 warnings. 13 are `unknownMacro` (cppcheck was not configured for the
`c_rbtree_for_each_entry*`/`c_list_for_each_entry*` iterator macros
from the vendored `libcstdaux`/`libcrbtree`/`libclist` subprojects) —
analyzer-configuration noise across production files, not defects.
6 `unknownEvaluationOrder` and 1 `danglingLifetime` are in
`subprojects/*/test-*.c` (intentional macro self-test / self-referential
`C_LIST_INIT`), ignore.
- One plausibly-real finding: `memleak` of `reply` on the
`USER_E_QUOTA`/`error_fold(r)` path in `src/bus/reply.c:64` (pending-
reply tracking). Worth a quick maintainer look; may be owned by a
registry that frees it later.
- `uninitvar` at `src/bus/peer.c:206` is a false positive (`r` is
assigned in the loop body; cppcheck modeled the zero-iteration case).
- Any significant Coverity results
- 47 defects (50 suppressed by filters). Most are in `test/` or
`subprojects/*/test-*.c` (ASSERT_SIDE_EFFECT, CONSTANT_EXPRESSION_RESULT,
OVERRUN in `libcshquote` tests that allocate `strlen(s)` for
`memcmp`-with-length, DC.WEAK_CRYPTO `rand()` in test binaries, etc.)
— ignore.
- Production findings worth noting (all likely false positives but on
security-critical paths):
- `src/util/selinux.c:125` UNINIT: `registry->n_refs` assigned without
`atomic_init` (SELinux context registry). The one plausibly-*real*
item; should use `atomic_init`/`c_refcount_init`.
- `subprojects/libcdvar-1/src/c-dvar-type.c:255` INTEGER_OVERFLOW with
array-index deref in the **type-signature parser** (parses untrusted
D-Bus type strings). Likely a Coverity modeling artifact (depth is
guarded), but highest-value manual-review target.
- `subprojects/libcrbtree-3/src/c-rbtree.c` (7× NULL_FIELD): backs the
policy/name-ownership/SELinux trees; RB-tree invariants normally
guarantee non-NULL siblings, likely FP.
- `src/broker/controller-dbus.c:173,440,686,739,779` and
`src/bus/driver.c:487` (6× Double free): the
`c_dvar_end_write`→`c_freep`→`c_dvar_deinit` cleanup idiom; almost
certainly FP (ownership transfers only on the success path), but on
the untrusted-D-Bus path — confirm `c_dvar_end_write` does not
populate `*datap` on its error return.
- `src/dbus/message.c:307` DEADCODE (defensive `default:` in the
header-field switch) and `:24,:562` `PW.STD_ALIGNOF_WITH_EXPR_ARG`
(GCC extension) — ignore.
- No genuine buffer-overflow, format-string, command-injection or
untrusted-pointer-deref was found in production code.
- Any significant shellcheck results
- none (clean).
- Any significant bandit results
- none (empty — no Python shipped beyond doc-build requirements).
- Any significant govulncheck results
- N/A (no Go code; scanner reports `no go.mod file`).
- Any significant Semgrep results
- none (empty).
(Flawfinder: 381 hits, none at severity 5; 53 at severity >=3, ~half in
test code. The reviewer-worthy shipped-code `strcpy` clusters in
`src/util/apparmor.c`, `src/bus/match.c`, `src/bus/policy.c`,
`src/dbus/address.c` are bounded by pre-computed `strlen+1` allocations;
`random()` in `src/bus/bus.c` and `src/launch/config.c` is sourced from
`AT_RANDOM` and non-security; `getenv` in `src/launch/launcher.c` is
trusted, read-only dir lookup. No exploitable pattern identified.
OSV-scanner: 2 distinct Medium CVEs in PyPI `idna` 3.9.0, only in the
docs-build `requirements.txt` of two vendored subprojects — do not ship
or run; bump to >=3.15 cosmetically.)
The 21-patch debian series (`original_series`) is dominated by AppArmor
work and is now in sync with Debian (comment #18). It enables real AppArmor
D-Bus mediation (previously stubbed out), ships two genuine bug fixes — a
NULL-pointer deref when a message has no interface (patch 0009,
`src/util/apparmor.c`) and a GID-based connect-policy keying bug
(`policy_at_gid()` was passed `->uid`, patch 0019) — and adds a defensive
audit-log fallback so AVC denials are visible without a running audit
daemon (patch 0021, using `"%s"`). The `c-util` subprojects are statically
linked and considered part of the dbus-broker release train per `NEWS.md`
("Any critical update to any subproject will cause a new release of
dbus-broker... Distributions are not required to monitor the subprojects
manually"), so the embedded sources are acceptable.
The previous blocker — `gdm` depending on `dbus-run-session` — is resolved
by the decision (comment #17/#25) to retain `src:dbus` in main (which
provides the policy/config files and `dbus-run-session`), so dbus-broker
can be promoted without demoting `src:dbus`.
Security team ACK for promoting dbus-broker to main, with the following
non-blocking notes:
1. Investigate `src/util/selinux.c:125` (Coverity UNINIT atomic refcount)
and `subprojects/libcdvar-1/src/c-dvar-type.c:255` (INTEGER_OVERFLOW
on the untrusted type-signature parse path). We will take a look more
closely on this one and confirmif real. Keep `libcdvar-1` and
`src/dbus/message.c` as priority fuzzing targets going forward.
2. Confirm the `c_dvar_end_write` ownership-transfer semantics
(controller-dbus.c / driver.c double-free pattern) are as expected.
3. It would be great if upstream could add a formal Security Policy /
coordinated-disclosure process (private disclosure was previously a
concern, comment #5).
4. Add `-fstack-clash-protection` to `dbus-broker-launch` (the broker
binary already has it).
5. Bump `idna` to >=3.15 in the two docs-only `requirements.txt` of the
vendored subprojects (cosmetic; does not ship or run).
Recommended TODOs:
- Consider migrating the launcher's `setgroups`/`setgid`/`setuid` to the
systemd-unit-based privilege drop (consistent with the MIR-team
recommendation in comment #1), keeping the current code as the
non-systemd fallback.
- Consider porting the AppArmor autopkgtest from `src:dbus` now that the
AppArmor mediation is bundled.
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/2015538
Title:
[MIR] dbus-broker
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/dbus-broker/+bug/2015538/+subscriptions
--
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
