[MIR] dbus-broker

Review for Package: src:dbus-broker

[Summary]
dbus-broker can be considered a drop-in replacement for dbus-daemon, the
reference implementation of the DBus protocol. It is supposed a more modern,
faster and safer alternative.

MIR team ACK

This does need a security review, so I'll assign ubuntu-security

List of specific binary packages to be promoted to main: dbus-broker
Specific binary packages built, but NOT to be promoted to main: None

Notes:
- This needs security review because of data-structure parsing, out-of-tree
  patch (see below), usage of setuid & embedded sources (which can be considered
  part of the project, though).

Required TODOs:
- None

Recommended TODOs:
- consider dropping privileges via systemd unit configuration instead of setuid
- dbus-broker is supposed to be a drop in replacement; try copying/migrating
  some autopkgtests from src:dbus (especially the apparmor one, as we will
  carry some related modifications)

[Duplication]
There is no other package in main providing the same functionality, BUT src:dbus
which we're trying replace.
Other (deprecated) alternatives include kdbus and bus1 kernel-space
implementations of the broker daemon or gdbus-daemon.

[Dependencies]
OK:
- no other Dependencies to MIR due to this
  - SRCPKG checked with `check-mir`
  - all dependencies can be found in `seeded-in-ubuntu` (already in main)
  - none of the (potentially auto-generated) dependencies (Depends
    and Recommends) that are present after build are not in main
- no -dev/-debug/-doc packages that need exclusion
- No dependencies in main that are only superficially tested requiring
  more tests now.

Problems: None

[Embedded sources and static linking]
OK:
- no static linking (but embedded subprojects, see below)
- does not have unexpected Built-Using entries
- not a go package, no extra constraints to consider in that regard
- not a rust package, no extra constraints to consider in that regard
- Includes vendored code, the package has documented how to refresh this
  code at: https://mesonbuild.com/Subprojects.html

Problems:
- embedded source present (several tiny C utils, which are not otherwise
  part of the Ubuntu archive): https://github.com/c-util
$ tree -d subprojects/ | grep "[libc|subprojects]"
subprojects/
├── libcdvar-1
│   ├── src
│   └── subprojects
├── libcini-1
│   ├── src
│   └── subprojects
├── libclist-3
│   └── src
├── libcrbtree-3
│   ├── src
│   │   └── docs
│   └── subprojects
├── libcshquote-1
│   ├── src
│   └── subprojects
├── libcstdaux-1
│   └── src
│   └── docs
└── libcutf8-1
    ├── src
    └── subprojects
21 directories

But, quoting ./NEWS.md (CHANGES WITH v30):
          All subprojects are still statically linked and considered part of
          dbus-broker. Any critical update to any subproject will cause a new
          release of dbus-broker, as it always did. Distributions are not
          required to monitor the subprojects manually."

[Security]
OK:
- history of CVEs does not look concerning
- does not run a daemon as root (it uses the "messagebus" user)
- does not use webkit1,2
- does not use lib*v8 directly
- does not process arbitrary web content
- does not use centralized...

While starting to work on preparing more for that transition we found an issue than gdm currently depends on dbus-daemon because it requires dbus-run-session to start greeter sessions
https://gitlab.gnome.org/GNOME/gdm/-/blob/main/daemon/gdm-session.c#L2973

there are discussion on the dbus-broker project about that gap
https://github.com/bus1/dbus-broker/issues/145

which also points to similar requirements in other components and desktops environment. It's unclear if we will be able to demote dbus-daemon / provide an equivalent solution in dbus-broker this cycle. Would be an acceptable option to maybe split dbus-run-session in its own binary built from dbus-daemon and keep installing that utility?

IMO the suggested solution is OK (for Mantic, as long as we're not doing it in an LTS). Though, the plan should still stay in place to get it fully transitioned to dbus-broker (demoting dbus-daemon) by the next LTS, so we don't have to maintain two DBus codebases for a long time.

The desktop team mentioned that they won't have capacity to re-implement 'dbus-run-session' as part of the dbus-broker codebase. So we need a decision between Foundations and Security if there's any capacity to get this done in Mantic, or alternatively maintain both 'dbus-daemon' and 'dbus-broker' packages for a longer time in parallel ...

If not the switch to dbus-broker might need to be dropped/postponed for the time being.

AFAIK, the dbus-run-session tool spawns dbus-daemon, so it wouldn't be possible to split it into its own binary package.

I partially reviewed dbus-broker 33-1 as checked into lunar. This shouldn't be considered a full audit but rather a quick gauge of maintainability.

> The dbus-broker project is an implementation of a message bus as defined by the D-Bus specification. Its aim is to provide high performance and reliability, while keeping compatibility to the D-Bus reference implementation. It is exclusively written for Linux systems, and makes use of many modern features provided by recent linux kernel releases.

- CVE History:
  - previous CVEs (CVE-2022-31212 and CVE-2022-31213) based on using untrusted input
    - https://sec-consult.com/vulnerability-lab/advisory/memory-corruption-vulnerabilities-dbus-broker/
    - upstream responded quickly to report
  - some security relevant commits
    - private disclosure is bad for downstream security maintenance
    - e.g., b08cc9dab51eab5a4b10c147ecd4cbd8efc238d6
 - upstream is using static analyzers and fuzzers with github actions \o/
 - upstream does not have a Security Policy
- Build-Depends?
  - debhelper-compat
  - libapparmor-dev
  - libaudit-dev
  - libcap-ng-dev
  - libdbus-1-dev
  - libexpat1-dev
  - libselinux1-dev
  - libsystemd-dev
  - linux-libc-dev
  - meson
  - pkg-config
  - python3-docutils
  - systemd
    - dbus-broker itself is not dependent on systemd, but dbus-broker-launch is
- pre/post inst/rm scripts?
  - adds or removes systemd services, reloads services, and requests reboots when needed
- init scripts?
  - none
- systemd units?
  - systemd service for main dbus-broker
  - owning team: check syslog for deprecation warnings that need to be fixed outside fo dbus-broker !
- dbus services?
  - minimizing global state and other design principals makes the security of dbus-broker more robust than dbus-daemon
    - see developers post https://dvdhrm.github.io/rethinking-the-dbus-message-bus/
- setuid binaries?
  - none
- binaries in PATH?
  - ./usr/bin/dbus-broker
  - ./usr/bin/dbus-broker-launch
- sudo fragments?
  - none
- polkit files?
  - none
- udev rules?
  - none
- unit tests / autopkgtests?
  - includes build tests
  - autopkgtests seem sparse
    - dbus programs like gnome-shell don't have autopkgtests
- cron jobs?
  - none
- Build logs:
  - looks okay

- Processes spawned?
  - only in launcher
    - for process and systemd
- Memory management?
  - **very** heavy use
  - cursory view looks o-k
- File IO?
  - use appears safe
- Logging?
  - written with logging mind
- Environment variable usage?
  - only XDG_* for launcher
- Use of privileged functions?
  - setuid/setgid/setgroups used to drop permissions
  - log_stream_send() contains a SIOCOUTQ ioctl for lossy D-Bus messages
- Use of cryptography / random number sources etc?
  - includes SASL wrapper to allow SASL D-Bus
- Use of temp files?
  - none
- Use of networking?
  - heavy socket use
  - heavily uses c-dvar for stream encoding/decoding D-Bus
    - https://github.com/c-util/c-dvar
    - same developer as dbus-broker
- Use of WebKit?
  - none
- Use of PolicyKit?
  - none

- Any significant cppcheck results?
  - nothing Coverity did not catch
- Any significant Coverity results?
  - reported upstream
  - upstream also runs https://sc...

Desktop packaged AppArmor features in the mantic version of dbus-broker \o/

Using a fresh mantic vm with dbus-broker [0], I ran dfuzzer on every dbus service (dfuzzer -l) and collected crash reproducers.

In another fresh mantic vm, running the default dbus-daemon, I reproduced most of these crashes.

An unprivileged user can access ~20% of the dbus services that user 1000 can.

Neither dbus nor dbus-broker have AppArmor profiles (apparmor_status).

apparmor.d is a set of +1400 AppArmor profiles [1][2]. Among its features is confining dbus (i.e., dbus-daemon). apparmor.d mitigated most crashes when a profile was available.

Snap does not currently confine dbus.

Confinement of dbus is a pre-existing issue and does not affect MIR outcome.

[0] https://github.com/bus1/dbus-broker
[1] https://github.com/roddhjav/apparmor.d
[2] https://www.youtube.com/watch?v=OzyalrOzxE8

I'm curious what the status is for writing a replacement wrapper to dbus-run-session.

https://github.com/bus1/dbus-broker/issues/145

There is no upstream traction and I don't think we have the resources to work on that from the Canonical Desktop Team perspective so we are stuck there for now

Thanks seb128

If you don't mind, I'll reach out to SEG to see if they have spare cycles.

Please do, we would still like to do the switch if possible

I'm inclined to move this to rls-oo-incoming in January.

I see that you are backporting the unmerged apparmor patch in a forked package - please feel free to send a MR on Salsa to add it https://salsa.debian.org/utopia-team/dbus-broker instead of forking the package.
That way we can keep the package in sync between Debian and Ubuntu.

Currently Ubuntu Noble is lagging behind, we have v35 in Debian. I'd really like to see v35 in Noble too, as it enables a new D-Bus interface for PIDFD tracking that we need in systemd.

We were already convinced by the former rationale, just no team found enough time to complete it so far.
But to add the reasoning for the same as recently discussed in arch - https://gitlab.archlinux.org/archlinux/rfcs/-/blob/master/rfcs/0025-dbus-broker-default.rst

And @Luca - thanks for the offer to help, I hope someone gets in touch with you.

@seb128 mentioned that there's also some activity on implemeting a `dbus-broker-session` https://github.com/bus1/dbus-broker/pull/321

Thanks to Christian the apparmor patch is now in Debian too, would be good to get 35-2 synced to Noble in time for the freeze

`dbus-run-session` needs to be implemented before `dbus` can be demoted and `dbus-broker` is promoted to main.

Vendored rust is not compatible with Debian, but it is allowed in Ubuntu packaging [0]. Possibly the current PR [1] can be used today. (`dbus-run-session` would need to be an alias for `dbus-broker-session`)

Having this promoted sooner than later is much better for testing, but _could_ be promoted between feature freeze and beta freeze.

[0] https://wiki.ubuntu.com/RustCodeInMain
[1] https://github.com/bus1/dbus-broker/pull/321

In Debian we just keep both, dbus-run-session from the src:dbus package works just fine, and it can be installed in parallel without any issues when somebody needs it. So it doesn't seem to me that you need that separate implementation?
The src:dbus package needs to remain in main anyway, as it provides the basic policy/configuration files/dependencies. This is by design.

I confirmed the delta in d/p/github_apparmor_support.patch is identical to what we have in Debian now and sync'ed the package.

Thank you Luca,
so a good next step for us would be to split the binary packages (if not already set up that way) to have one for dbus-run-session, one package for the policy/config/deps that you mentions (those two we'd keep in main), and one package (for universe) for the daemon that we want to demote.

Thanks sarnold for the suggestion.

Back to Seb128 I guess to consider this from the Desktop POV?
This looks much closer to being doable now AFAICS.

@seb128, could you please review the recent discussion?

Sorry for the lack of reply, I've been busy and now I'm out sick probably for a few days, anyway quick notes

The plan suggested sounds reasonable but

- the dbus-broker patch is still under review upstream and got not real world testing which means it's difficult to have confidence it's working to work without issues

- it is adding rust to the package which makes it more difficult to cherrypick/package/MIR

- the timeline doesn't feel realistic for the LTS, that's not a trivial change and not something we can land that late in the cycle especially when the archive is still not in working shape due to the time_t transition (which impacts our capacity to tests feature work that landed and get feedback)

We could do the package split but I don't see the point of rushing doing that work before the LTS if we don't do the switch, I would prefer for us to work with Debian on those changes and bring them back to Ubuntu next cycle

Does it make sense to others?

Thank you @seb128. I was asked to get your feedback before completing the Security review. Get well soon!

Security team ACK for promoting dbus-broker to main, under the condition that src:dbus' binary packages are split as described by @paelzer in comment #19.

@seb128 IIUC comment #17, we do not necessarily need the new Rust implementation, but rather "just" split dbus-run-session out of the "dbus-daemon" binary package. So it can be installed individually and we can demote the (pure) dbus-daemon package to universe and replace it with dbus-broker in main.

Errata: I talked with smcv, who explained to me that dbus-run-session is actually a wrapper around dbus-daemon itself, so they are not independent.

With user sessions managed by logind&al, what is the use case for dbus-run-session in production? I am aware it is used for self-contained tests and such things, but using depending on the dbus-daemon package should be fine for that. Anything else?

@Luca, thanks, that confirms what I've been told before and pointed out during the previous MIR meeting, dbus-run-session has an hard depends on dbus-daemon so the split idea isn't possible

About what makes us rely on dbus-run-session is currently gdm (which smcv also mentioned on IRC), basically
https://gitlab.gnome.org/GNOME/gdm/-/blob/main/daemon/gdm-session.c#L3195

It does sound like we are blocked for now for that switch

Copying some extra details from that IRC conversation

> until gdm learns to generate one system user per greeter on multi-seat systems, each with their own `systemd --user`, which I think has been the upstream plan for several years

IMHO it would be a worthwhile investment to allocate resources to implement that plan, as it would benefit desktop users for all cases, not just with dbus-broker