Bug #1811824 “[MIR] xdg-dbus-proxy” : Bugs : xdg-dbus-proxy package : Ubuntu

Jeremy Bícha (jbicha) on 2019-02-19

description:

updated

Revision history for this message

Jeremy Bícha (jbicha) wrote on 2019-02-25:

#1

The new feature has been delayed until webkit2gtk 2.26 to be released in September 2019.

description:	updated
tags:	removed: cosmic

Revision history for this message

Didier Roche-Tolomelli (didrocks) wrote on 2019-04-09:

#2

Please set it back to New once the MIR team can have a look at it.

Changed in xdg-dbus-proxy (Ubuntu):
status:	New → Incomplete

Revision history for this message

Jamie Strandboge (jdstrand) wrote on 2019-05-10:

#3

Download full text (9.8 KiB)

It seems the security team was no longer assigned, but we knew this was
coming so I performed the security team MIR.

I reviewed xdg-dbus-proxy 0.1.1-1 as checked into eoan. This shouldn't be
considered a full audit but rather a quick gauge of maintainability.

xdg-dbus-proxy is a "filtering proxy for D-Bus connections". It is invoked via
the command line line with arguments that specify the address to proxy and the
path for client connections. It will listen on a unix domain socket for
clients to connect. For each client connection, it opens a new connection to
the proxied address, forwarding data between the two. xdg-dbus-proxy operates
in two modes, filtered and unfiltered (ie, forward all messages). With
filtering mode, policy is applied to determine which messages to allow and
drop. Filtering is applied to signals and method calls from the client and
broadcast signals from the proxied address. Replies (errors and method returns)
are allowed for the outstanding call, but not otherwise (this is analogous to
how dbus-daemon works with LSM policy like AppArmor).

Policy is simple, yet flexible and aggregates various common DBus APIs into
SEE, TALK and OWN policy levels (listed from lowest to highest, with each
higher level implying lower levels) that are specified in rules for use with
well-known DBus names (ie, *not* private/unique names). The default policy
allows TALKing to the bus itself (org.freedesktop.DBus) and to its own unique
ID. Other clients are invisible (this is not unlike the dbus-strict and
dbus-session-strict AppArmor abstractions). Policy rules allow a glob syntax
for suffixes such that org.foo.* matches org.foo, org.foo.bar, org.foo.bar.baz
but not org.norf. While the rule is specified for the well-known name, the rule
will also apply such that the policy for the private/unique DBus name of the
client is the union of all rules for well-known names that the client owns.
where the highest level wins. In addition to these policy levels, it is
possible to also specify interface and object paths with method calls/signals
on well-known names (similar to fine-grained AppArmor rules).

xdg-dbus-proxy is a lowlevel application intended to be driven by a higher
level application (eg, bubblewrap, flatpak (which drives bubblewrap, etc). It
does not support policy files and typical usage is to proxy, for example, the
session bus for an application, with all rules specified on the command line
(bwrap typically uses the --args option, which allows passing nul-separated
arguments via a file descriptor).
Eg (showing full args instead of the normal --args usage):

  $ xdg-dbus-proxy --fd=26 unix:path=/run/usr/1000/bus
  /run/usr/1000/.dbus-proxy/session-bus-proxy --filter --own=org.gnome.ghex.*
  --talk=ca.desrt.dconf --call=org.freedesktop.portal.*=*

such that different applications will each have their own proxy.

bwrap usage shows sockets in /run/user/<uid>/.dbus-proxy/session-bus-proxy-*
where there is a named socket allocated for each application that corresponds
to to the (now deleted) --args fd. xdg-dbus-proxy does support proxying the
system bus (eg, xdg-dbus-proxy unix:path=/run/dbus/system_bus_socket /tmp/foo).
Additionally, wi...

It seems the security team was no longer assigned, but we knew this was
coming so I performed the security team MIR.

I reviewed xdg-dbus-proxy 0.1.1-1 as checked into eoan.  This shouldn't be
considered a full audit but rather a quick gauge of maintainability.

xdg-dbus-proxy is a "filtering proxy for D-Bus connections". It is invoked via
the command line line with arguments that specify the address to proxy and the
path for client connections.  It will listen on a unix domain socket for
clients to connect. For each client connection, it opens a new connection to
the proxied address, forwarding data between the two. xdg-dbus-proxy operates
in two modes, filtered and unfiltered (ie, forward all messages). With
filtering mode, policy is applied to determine which messages to allow and
drop. Filtering is applied to signals and method calls from the client and
broadcast signals from the proxied address. Replies (errors and method returns)
are allowed for the outstanding call, but not otherwise (this is analogous to
how dbus-daemon works with LSM policy like AppArmor).

Policy is simple, yet flexible and aggregates various common DBus APIs into
SEE, TALK and OWN policy levels (listed from lowest to highest, with each
higher level implying lower levels) that are specified in rules for use with
well-known DBus names (ie, *not* private/unique names). The default policy
allows TALKing to the bus itself (org.freedesktop.DBus) and to its own unique
ID. Other clients are invisible (this is not unlike the dbus-strict and
dbus-session-strict AppArmor abstractions). Policy rules allow a glob syntax
for suffixes such that org.foo.* matches org.foo, org.foo.bar, org.foo.bar.baz
but not org.norf. While the rule is specified for the well-known name, the rule
will also apply such that the policy for the private/unique DBus name of the
client is the union of all rules for well-known names that the client owns.
where the highest level wins. In addition to these policy levels, it is
possible to also specify interface and object paths with method calls/signals
on well-known names (similar to fine-grained AppArmor rules).

xdg-dbus-proxy is a lowlevel application intended to be driven by a higher
level application (eg, bubblewrap, flatpak (which drives bubblewrap, etc). It
does not support policy files and typical usage is to proxy, for example, the
session bus for an application, with all rules specified on the command line
(bwrap typically uses the --args option, which allows passing nul-separated
arguments via a file descriptor).
Eg (showing full args instead of the normal --args usage):

$ xdg-dbus-proxy --fd=26 unix:path=/run/usr/1000/bus
  /run/usr/1000/.dbus-proxy/session-bus-proxy --filter --own=org.gnome.ghex.*
  --talk=ca.desrt.dconf --call=org.freedesktop.portal.*=*

such that different applications will each have their own proxy.

bwrap usage shows sockets in /run/user/<uid>/.dbus-proxy/session-bus-proxy-*
where there is a named socket allocated for each application that corresponds
to to the (now deleted) --args fd. xdg-dbus-proxy does support proxying the
system bus (eg, xdg-dbus-proxy unix:path=/run/dbus/system_bus_socket /tmp/foo).
Additionally, with typical bwrap usage, xdg-dbus-proxy isn't going to be
invoked as root as it is intended for use in user sessions. This architecture
also seems reasonable.

The rules language, syntax and xdg-dbus-proxy invocation looks fine from a
design POV. Due to time constraints, I did not review the implementation for
enforcement correctness. I did some blackbox testing like so:

$ sudo apt-get install flatpak gnome-software-plugin-flatpak d-feet
$ flatpak remote-add --if-not-exists flathub https://flathub.org/repo/flathub.flatpakrepo
$ flatpak install flathub org.gnome.ghex
$ flatpak install flathub org.gnome.Logs
$ flatpak install flathub org.wireshark.Wireshark
$ flatpak run org.gnome.ghex &
$ flatpak run org.gnome.Logs &
$ flatpak run org.wireshark.Wireshark &
$ xdg-dbus-proxy unix:path=/run/dbus/system_bus_socket /tmp/test-system & # unfiltered
$ xdg-dbus-proxy unix:path=/run/dbus/system_bus_socket /tmp/test-session & # unfiltered

$ d-feet # 'Connect to other bus', choose unix:path=/tmp/test-system and see system bus
$ d-feet # 'Connect to other bus', choose unix:path=/tmp/test-session and see session bus
$ ls -l /run/user/1000/.dbus-proxy/
total 0
srwxr-xr-x 1 jamie jamie 0 May 10 17:32 a11y-bus-proxy-A5PT1Z
srwxr-xr-x 1 jamie jamie 0 May 10 16:35 a11y-bus-proxy-ORKL1Z
srwxr-xr-x 1 jamie jamie 0 May 10 16:36 a11y-bus-proxy-PZFD1Z
srwxr-xr-x 1 jamie jamie 0 May 10 16:35 session-bus-proxy-9IQL1Z
srwxr-xr-x 1 jamie jamie 0 May 10 16:36 session-bus-proxy-EZ9C1Z
srwxr-xr-x 1 jamie jamie 0 May 10 17:32 session-bus-proxy-KHRT1Z

(interesting that these all end with '1Z', but this would be in bwrap)

$ d-feet # 'Connect to other bus', choose unix:path=/run/user/1000/.dbus-proxy/session-bus-proxy-9IQL1Z

In this last example, we see only what ghex is allowed (verified by comparing
d-feet with $ flatpak info --show-permissions org.gnome.ghex, eg, it has dconf
and a few other things, but not the whole session). Do the same with Logs and
wireshark (which doesn't have dconf which is correct per flatpak info). Poked
around and indeed other access are denied.

- CVE History:
  - CVE-2018-6560 (when still part of flatpak proper) which was fixed in
    https://github.com/flatpak/flatpak/commit/52346bf187b5a7f1c0fe9075b328b7ad6abe78f6.
    This was fixed in a timely manner, though the code changes were a little
    more than expected because it was updated to match the authentication phase
    of the dbus-daemon code. The CVE demonstrates the fragility of the proxy
    wrt dbus-daemon protocols and the proxy curiously failed open rather than
    closed.
- Build-Depends
  - nothing terribly interesting: libglib2.0-dev, xsltproc, and dbus
- pre/post inst/rm scripts
  - None
- init scripts
  - None
- systemd units
  - None
- dbus services
  - None
- setuid binaries
  - None
- binaries in PATH
  - /usr/bin/xdg-dbus-proxy
- sudo fragments
  - None
- udev rules
  - None
- unit tests / autopkgtests
  - unit: source has unit tests in ./tests and they are enabled during the build. Unit tests are shallow
  - autopkgtest: packaging provides xdg-dbus-proxy-tests which is callable via gnome-desktop-testing-runner. Ships autopkgtests that call these tests in this manner. This is simply the shallow test-proxy from the unit tests
- cron jobs
  - None
- Build logs:
  - no warnings or errors
  - there are lintian warnings and errors but they aren't security relevant:
    W: xdg-dbus-proxy source: debhelper-compat-file-is-missing
    W: xdg-dbus-proxy source: package-uses-deprecated-debhelper-compat-version 1
    E: xdg-dbus-proxy source: package-uses-debhelper-but-lacks-build-depends
    E: xdg-dbus-proxy source: missing-build-dependency debhelper
    W: xdg-dbus-proxy source: newer-standards-version 4.3.0 (current is 4.1.4)
- Processes spawned
  - None
- Memory management - this isn't exhaustive but a spot check on code quality
  - uses g_* from glib primarily
  - it does implement its own buffer_new() convenience function which itself
    uses g_malloc0 and a g_assert() prior to a memcpy(). There is the magic '16'
    value in buffer_new() that corresponds to the guchar 'data' field and this
    magic value is used in several places in flatpak-proxy.c. The handling of
    this data field in the struct seems a bit clever, but not wrong AFAICS.
  - another use of memcpy() in message_to_buffer() is fine since it happens
    after the call to buffer_new()
  - the final use of memcpy() happens after use of the glib MIN macro with a
    resizing via glib functions as needed after
- File IO
  - None
- Logging
  - just via standard glib functions
- Environment variable usage
  - None in main code; test-proxy.c uses g_getenv() and correctly checks for
    NULL
- Use of privileged functions
  - None
- Use of cryptography / random number sources etc
  - None
- Use of temp files
  - Only during build via autotools
- Use of networking
  - no networking
  - all local unix domain sockets (see flatpak_proxy_start() which uses
    g_unix_socket_address_new()). Communication with DBus happen via g_dbus_*.
- Use of WebKit
  - None
- Use of PolicyKit
  - None
- Any significant cppcheck results
  - None
- Any significant Coverity results
  - None

In terms of code quality, the source is easy to read and coded defensively. The
use of glib is nice. Spot-checking revealed nothing shocking.
parse_generic_args() in dbus-proxy.c doesn't do range checks with strtol and
returns an int rather than a long, but the subsequent checks are sufficient
since if LONG_MAX is returned, fd is negative and the program aborts. In
general, xdg-dbus-proxy is using glib which guards against a number of common
problems. Also see Memory Management, above. The xdg-dbus-proxy has man page
with a nice overview of how it operates.

Blackbox testing confirms the design though I wouldn't have minded more time to
review the policy and enforcement code (but the code quality of what I looked
at and the activity from upstream demonstrates it is suitable for main). The
CVE gives me some concern since the proxy failed open in the face of unexpected
data in the authentication phase. It was updated to match upstream dbus-daemon.
dbus-daemon and its implementation of the dbus protocol is very mature, but I'm
a bit worried that changes in dbus might introduce other issues when the proxy
and the dbus daemon have different ideas on the protocol. The proxy was updated
in a timely manner and I don't expect the protocol to change. While I have
concern, considering upstream's reaction to the issue, that concern does not
outweigh the code quality on the whole or the benefits of using xdg-dbus-proxy
via bwrap to harden the massive webkit2gtk code base.

Security team ACK for promoting xdg-dbus-proxy to main. It would be nice if the
unit and autopkgtests were expanded since there are a lot of testing
opportunities, but this is not a requirement for promotion.

Sebastien Bacher (seb128) on 2019-05-28

Changed in xdg-dbus-proxy (Ubuntu):
status:	Incomplete → New

Ken VanDine (ken-vandine) on 2019-05-28

Changed in xdg-dbus-proxy (Ubuntu):
assignee:	nobody → Ken VanDine (ken-vandine)

Sebastien Bacher (seb128) on 2019-08-21

Changed in xdg-dbus-proxy (Ubuntu):
assignee:	Ken VanDine (ken-vandine) → nobody

Revision history for this message

Christian Ehrhardt  (paelzer) wrote on 2019-08-21:

#4

starting a MIR review after discussing with seb and cleaning up what made it disappear from the lists that we check.

Changed in xdg-dbus-proxy (Ubuntu):
assignee:	nobody → Christian Ehrhardt  (paelzer)

Revision history for this message

Christian Ehrhardt  (paelzer) wrote on 2019-08-21:

#5

[Duplication]
No other comparable filtering dbus proxy in the archive (and none in main obviously)

[Embedded sources and static linking]
- no embedded sources
- no golang
- no static linking

[Security]
- no CVEs on top of the reported and fixed CVE-2018-6560
- does not run a daemon as root
- does not uses webkit1,2 (in fact only becomes important with webkit2gtk 2.26)
- does not use lib*v8 directly
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop

It does
- parses data formats
- opens a port (not a classic one but on dbus)
- does not processes arbitrary web content, but semi arbitrary dbus messages
- does not deals with system authentication (eg, pam), etc), but dbus is involved there and due to that it is as well.

This needs a security review, which fortunately already was done.
So we can feel safe on that side.

[Common blockers]
- builds on all arches without FTBFS
- has a (minimal) build time test
- has a autopkgtest
- code/msg isn't really user visible (no translations needed)
- not a python package
- Desktop Packages is subscribed
- Desktop packages is subscribed

[Packaging red flags]
- no Ubuntu delta
- no library that is exposed for symbols tracking
- watch file present
- update history seems ok
- current release packaged
- no MOTU conflict
- no massive Lintian warnings
- debian/rules is small and clean
- no Built-Using
- no golang checks needed

[Upstream red flags]
- no warning/errors on build
- no incautious use of malloc/sprintf that came up in checkers
- no use of sudo, gksu, pkexec, or LD_LIBRARY_PATH
- no use of user nobody
- no use of setuid
- no known Important bugs (crashers, etc) in Debian or Ubuntu
- no Dependency on webkit, qtwebkit, seed or libgoa-* (but vice versa)
- no Embedded source copies

[Summary]
This package seems fine - ACK for the MIR Team

Changed in xdg-dbus-proxy (Ubuntu):
status:	New → Fix Committed
assignee:	Christian Ehrhardt  (paelzer) → nobody

Revision history for this message

Sebastien Bacher (seb128) wrote on 2019-08-21:

#6

Override component to main
xdg-dbus-proxy 0.1.1-1 in eoan: universe/misc -> main
xdg-dbus-proxy 0.1.1-1 in eoan amd64: universe/admin/optional/100% -> main
xdg-dbus-proxy 0.1.1-1 in eoan arm64: universe/admin/optional/100% -> main
xdg-dbus-proxy 0.1.1-1 in eoan armhf: universe/admin/optional/100% -> main
xdg-dbus-proxy 0.1.1-1 in eoan i386: universe/admin/optional/100% -> main
xdg-dbus-proxy 0.1.1-1 in eoan ppc64el: universe/admin/optional/100% -> main
xdg-dbus-proxy 0.1.1-1 in eoan s390x: universe/admin/optional/100% -> main
Override [y|N]? y

Changed in xdg-dbus-proxy (Ubuntu):
status:	Fix Committed → Fix Released

Ubuntu
xdg-dbus-proxy package

[MIR] xdg-dbus-proxy

Bug Description

CVE References

Other bug subscribers

Remote bug watches

Ubuntuxdg-dbus-proxy package

[MIR] xdg-dbus-proxy

Bug Description

CVE References

Other bug subscribers

Remote bug watches

Ubuntu
xdg-dbus-proxy package