Check for D-Bus session bus name is too restrictive

snapd
1

As part of wrapping transmission-gtk in snap, I’ve stumbled upon an issue with it being unable to register with the following error:

Failed to register: GDBus.Error:org.freedesktop.DBus.Error.AccessDenied: Connection “:1.152” is not allowed to own the service “com.transmissionbt.transmission_2049_1191701” due to AppArmor policy

The name “com.transmissionbt.transmission_2049_1191701” contains two numbers: device ID and inode of the configuration directory, and these are being appended to guarantee that no two instances of transmission-gtk are using the same configuration directory; note here that running two transmission-gtk instances using different configuration directories (say, under different user accounts) is perfectly fine (which could still be an issue depending on the configuration itself but is easily resolved and will never lead to files corruption).

As has been demonstrated to me, the check currently in place only allows dash as a separator between the name and the digits, and then only allows up to 7 digits. This is in place to allow to e.g. append process IDs to the bus name and thus be able to guarantee the process uniqueness globally. In relation to transmission-gtk this is too restrictive and even if I change the format of the name to use hyphens instead of underscores there will still be an issue of 7 digits not being enough, and the issue of not being able to separate the digits so that diferent IDs are treated as equal (e.g. device ID 11 and inode ID 22, and device ID 1 and inode ID 122).

I hereby kindly request to somehow extend the validation to support wider range of bus name formats while still preserving security of those names.

CC @jdstrand

2

I personally think it would be acceptable to include underscores and hyphens in the matching. I propose the following regular expression where the bus name is ###DBUS_NAME### (not a valid apparmor rule):

/###DBUS_NAME###[-_][1-9][0-9-_]*/
3

We should not just add ‘_’ to the policy because ‘_’ is used as part of the escape sequence for characters that aren’t allowed in the DBus specification for well-known names. Eg: the security label ‘snap.foo.bar’ gets translated to ‘snap_2efoo_2ebar’ when performing mediation. With parallel installs of foo_baz, the security label is ‘snap.foo_baz.bar’ gets translated to ‘snap_2efoo_5fbaz_2ebar’. I’d prefer leaving ‘_’ out of the dbus.go generic policy as a hardening measure to avoid introducing the possibility of overlapping policy.

I see two paths forward:

  1. we create a dedicated ‘transmission’ interface that allows these accesses
  2. the transmission snap is modified to use ‘-’ instead of ‘_’ and we adjust the policy to allow something more like what transmission is used to

I prefer ‘1’ since the dbus interface is not meant to address every DBus application out there, but instead typical ones (indeed, the current allowance of -[0-9]+ is a compromise for KDE applications; let’s not compromise on the compromise :wink: ).

Looking at transmission’s website, it seems like this is an ok fit as well since there is the possibility a headless server being controlled by a client. In this configuration, the server would slot the transmission snapd interface with client snaps able to plug the transmission snapd interface to connect to the server’s DBus APIs. @mike.dld - please correct me if that assessment is wrong.

Someone interested in implementing the transmission interface could look at interfaces/builtin/thumbnailer{,_test}.go for inspiration.

4

GTK+ and Qt Transmission clients use D-Bus only to connect to their own self, i.e. to check whether an existing instance using the specific configuration directory is already running and to forward whatever torrent files or URLs were passed to it up to the existing instance. This D-Bus interface is not meant to be used by any external clients; it would also be unreliable to do so given that the algorithm used to generate the bus name is not documented and thus could change any time. For client-server communication, Transmission has a JSON RPC interface.

Introducing a Transmission-specific interface (or anything else really) sounds a bit over the top to me. I can’t say for sure that there exist other programs that construct the bus name in “KDE-incompatible” way but I’m still for option no. 2 (combined with someting like what you and @lucyllewy mentioned, i.e. to basically allow (-[0-9]+)* suffix) unless there’re some real concerns about it. Taking the clarification above into account, it would not be a problem for Transmission to adjust the bus naming scheme and use - instead of _ to separate the numbers. If you still want to limit the freedom there, it would be possible to accomodate both KDE and Transmission by allowing just the (-[0-9]+){,2} suffix (with at most two numbers).

With all that in mind, I’d also like to point out that D-Bus spec explicitly mentions that - chars are “discouraged in new bus names”:

Note that the hyphen (’-’) character is allowed in bus names but not in interface names. It is also problematic or not allowed in various specifications and APIs that refer to D-Bus, such as Flatpak application IDs, the DBusActivatable interface in the Desktop Entry Specification, and the convention that an application’s “main” interface and object path resemble its bus name. To avoid situations that require special-case handling, it is recommended that new D-Bus names consistently replace hyphens with underscores.

Just thought I’d mention that. Would probably mean that if we settle on - as a separator Transmission code is likely to end up having special handling for Snap environment.

5

Sure, but the snapd dbus.go interface supports more than just that, but see below.

Well, snapd is in the business of creating specific interfaces when they make sense. Based on your comments, it does sound like it is not appropriate for this interface.

This is a compelling argument for snapd to not force an application to go against best practices and I agree that it would be awkward if we forced Transmission to do this.

In thinking through this more, I think we should let the _ in the latter bits. Unfortunately, the apparmor syntax does not allow for regexs, so we’ll need to craft the new rule accordingly. Something along the lines of: name=###DBUS_NAME###{_,-}[1-9]{,[0-9_]}{,[0-9_]}{,[0-9_]}{,[0-9_]}{,[0-9_]}{,[0-9_]}{,[0-9_]}{,[0-9_]}{,[0-9]}{,_[1-9]{,[0-9_]}{,[0-9_]}{,[0-9_]}{,[0-9_]}{,[0-9_]}{,[0-9_]}{,[0-9_]}{,[0-9_]}{,[0-9]}},

This allows the old -123456 (with more digits) as well as _<unsigned long>_<unsigned long>. This continues to be safe so long as ###DBUS_NAME### is the full name of the snap (though if the dbus interface were expanded to support parallel installs, there is an opportunity for the parallel install and the original install to overlap with this rule (we’ll need a comment in the code for that)).

I’ve queued this up for the 2.37 batch of profile updates.