Bug 101902

Summary: Containers (#100344): message filtering/policy
Product: dbus Reporter: Simon McVittie <smcv>
Component: coreAssignee: Simon McVittie <smcv>
Status: RESOLVED MOVED QA Contact: D-Bus Maintainers <dbus>
Severity: enhancement    
Priority: medium CC: alexl, bugzilla, dbus, desrt, james
Version: git master   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:
Bug Depends on: 101354, 105656, 105657, 105658, 105659, 105660    
Bug Blocks: 100344, 105661    

Description Simon McVittie 2017-07-24 19:01:54 UTC
+++ This bug was initially created as a clone of Bug #100344 +++

Bug #101354 does not let container managers arrange for messages to be filtered. This is part of the wish-list for Bug #100344, because it would eventually let us get rid of flatpak-dbus-proxy.

Design sketch:

* New named parameter:

      Allow: a(usos)
          Array of tuples (flags: u, bus name: s, object path: o, interface: s)

          If this parameter is given, it sets the messages and operations
          that are allowed.

          An empty array allows sending method calls to the message
          bus, and receiving exactly one reply or error for each method call
          successfully sent (unless it was NO_REPLY). It also allows
          receiving method calls from connections that are not in a container,
          and sending exactly one reply or error for each method call
          received (unless it was NO_REPLY). It does not allow receiving
          broadcast signals. This is the most restrictive possible policy.

          [TODO: Does it allow receiving unicast signals from connections
          that are not in a container?]

          Each array entry is a /rule/ which allows additional operations
          according to its flags. If the bus name is the empty string,
          it matches all bus names (and hence all possible connections).
          If the interface is the empty string, it matches messages with
          any interface or no interface. If the object path is "/" and
          the OBJECT_PATH_IS_SUBTREE flag is present, it matches messages
          with any object path or no object path.

              SEE:
                  The connection may call GetNameOwner(), NameHasOwner(),
                  GetConnectionCredentials(), etc. to inspect connections
                  that are in the queue to own the given bus name.
                  The object path and interface are ignored when checking
                  for SEE access.
              CALL_METHODS:
                  The connection may send method call messages to
                  connections that are in the queue to own the given
                  bus name. Implies SEE.
              RECEIVE_BROADCASTS (or RECEIVE_SIGNALS?):
                  The connection may receive broadcasts (or all signals?)
                  from connections that are in the queue to own the given
                  bus name, if they match the given object path and
                  interface.
              TALK: alias for SEE | CALL_METHODS | RECEIVE_whatever
              BUS_NAME_IS_SUBTREE:
                  The bus name is matched in the same way as arg0namespace
                  in signal match rules, instead of as an exact match.
              OBJECT_PATH_IS_SUBTREE:
                  In addition to messages that match exactly, the object
                  path matches messages with an object path that has
                  additional components. For example, "/foo" matches messages
                  with object path "/foo" or "/foo/bar" but not "/foolish".
                  Note that this is not identical to arg0path in signal
                  match rules.
              SEND_UNIX_FDS, RECEIVE_UNIX_FDS:
                  The connection may send, receive messages that contain
                  Unix fds and match the given bus name, object path and
                  interface.
              OWN_BUS_NAME:
                  The connection may use RequestName and ReleaseName
                  to own the given bus name. Implies SEE and TALK.
                  The object path must be "/" and the interface must
                  be "".

          Not giving this parameter is equivalent to providing policies
          that allow the confined connection to send and receive every
          message, own every bus name, etc. This is the least restrictive
          possible policy.

* o.fd.Containers.GrantAccess(a(uos): rules)
      Rules are as above, but bus name is implicitly the caller's
      unique name

* o.fd.Containers.RevokeAccess(a(uos): rules)
      Rules are as for GrantAccess. If a rule was added more than
      once, each revocation removes one of the identical copies.
Comment 1 Simon McVittie 2017-07-24 20:06:32 UTC
We also need to either allow all confined apps to send broadcast signals, or have a flag that lets them.

We also need to either allow them to send unicast signals, or have a flag that lets them, or consider it to be part of CALL_METHODS (perhaps renamed to SEND_UNICAST in that case).
Comment 2 Philip Withnall 2017-08-01 11:31:27 UTC
(In reply to Simon McVittie from comment #0)
>           [TODO: Does it allow receiving unicast signals from connections
>           that are not in a container?]

Yes, that’s used by a few system service implementations, right?
Comment 3 Simon McVittie 2017-10-26 16:56:04 UTC
For feature parity with Flatpak, we need the equivalents of:

* flatpak-dbus-proxy with no additional arguments

* flatpak-dbus-proxy --filter --own=APP_ID --own=APP_ID.*
  [--own=NAME[.*]...] [--talk=NAME[.*]...]

* flatpak-dbus-proxy --filter --sloppy-names 
  --filter=org.a11y.atspi.Registry=IFACE.METHOD@PATH

flatpak-dbus-proxy also has --see=NAME[.*] but Flatpak never actually uses that, as far as I can tell. (However, the "see" policy can happen implicitly at runtime, because whenever a peer sends a message to the container, flatpak-dbus-proxy gives the container SEE access to that peer.)

flatpak-dbus-proxy also has --log but that's Bug #103470.
Comment 4 Simon McVittie 2017-10-26 17:46:19 UTC
Feature parity with flatpak-dbus-proxy:

Levels of access in Flatpak: NONE < SEE < FILTERED < TALK < OWN. Access to a unique name is always >= the level of access we have to any well-known name that it owns. Unlike dbus-daemon <allow> rules, well-known names that it is merely queueing for don't count (but that's probably just because Flatpak can't know that).

Unexpected replies are always blocked, in both directions. Expected replies are always allowed, in both directions.

Sending broadcast signals is always allowed.

Peer-to-peer messages to the dbus-daemon are always allowed. In practice, the dbus-daemon only handles Peer messages here (anything else has to have o.fd.DBus as destination).

Directed messages (method calls or unicast signals) to connections other than o.fd.DBus for which the container has TALK or OWN access are always allowed.

Directed method calls (only - no unicast signals) sent to connections for which we have FILTERED access are allowed if they match the filter.

Introspecting o.fd.DBus is always allowed. So are Hello, RemoveMatch and GetId.

Special case for AddMatch: it is allowed, if it isn't eavesdropping.

UpdateActivationEnvironment and BecomeMonitor are never allowed.

RequestName, ReleaseName and ListQueuedOwners are allowed iff we have OWN access to the name.

Special case for ListActivatableNames, ListNames, NameHasOwner, GetNameOwner, GetConnectionWhatever: names to which we don't have SEE access are invisible.

Special case for StartServiceByName: it requires TALK access (in particular FILTERED access is not enough).

Special case for NameOwnerChanged: only names to which we have >= SEE access are listed, but --sloppy-names is equivalent to having SEE access to every unique name. If a client has >= SEE access to a well-known name, and receives NameOwnerChanged for that well-known name, then that client (but not other clients in the same proxy?) is granted SEE access to the old and new owners as well.

Other dbus-daemon methods and interfaces are blocked.

Receiving a method call or a unicast signal from a peer grants the recipient (but no other client of the same proxy) SEE access to that peer. I think we can relax that to giving everything in the same container instance SEE access to that peer, because there is no security boundary within a container instance.

Receiving broadcast signals from outside the container instance checks for TALK access to the sender.

flatpak-dbus-proxy users can always TALK to their own unique name. Because OWN implies TALK, they can also TALK to anything in the same container instance, as long as communication was initiated using the well-known name. I think it would be reasonable to relax this to say that talking to other unique names in the same container instance is fine, because there is no security boundary within a container instance.
Comment 5 Simon McVittie 2017-10-26 17:53:40 UTC
(In reply to Simon McVittie from comment #3)
> * flatpak-dbus-proxy with no additional arguments

That's a call to AddServer() without having Allow in the named parameters at all. Technically, for full parity with Flatpak, it should also have privileged access (ability to escape the container or spy on other processes by talking to dbus-daemon) re-enabled (Bug #103458).

> * flatpak-dbus-proxy --filter --own=APP_ID --own=APP_ID.*
>   [--own=NAME[.*]...] [--talk=NAME[.*]...]

The equivalent of --filter can be activated by having the Allow parameter at all. We will need to divide up the "always allowed" rules from Flatpak into things that are always allowed, and things that Flatpak allows by adding rules.

The equivalent of --talk=NAME is (flags=SEE|CALL_METHODS|SEND_UNICAST_SIGNALS|RECEIVE_BROADCASTS|SEND_UNIX_FDS|RECEIVE_UNIX_FDS|OBJECT_PATH_IS_SUBTREE|ACTIVATE, bus_name=NAME, object_path=/, interface=""). (New flags needed here since Comment #0: SEND_UNICAST_SIGNALS, ACTIVATE.)

If NAME ends with ".*", then remove that and add BUS_NAME_IS_SUBTREE.

The equivalent of --own=NAME is the same as --talk, but add OWN_BUS_NAME to the flags.

> * flatpak-dbus-proxy --filter --sloppy-names 
>   --filter=org.a11y.atspi.Registry=IFACE.METHOD@PATH

The equivalent of --sloppy-names would need to be a new flag (either an Allow rule with the flag or a separate named parameter), SEE_ALL_UNIQUE_NAMES or SeeAllUniqueNames.

We need a new flag INTERFACE_IS_REALLY_METHOD, then the equivalent of that --filter rule would be (flags=SEE|CALL_METHODS|SEND_UNIX_FDS|RECEIVE_UNIX_FDS|INTERFACE_IS_REALLY_METHOD, bus_name=org.a11y.atspi.Registry, object_path=PATH, interface=IFACE.METHOD). I don't think it's worth adding a new top-level struct member for this: filtering at method granularity is hopefully really rare (interface granularity is better).

> flatpak-dbus-proxy also has --see=NAME[.*] but Flatpak never actually uses
> that, as far as I can tell. (However, the "see" policy can happen implicitly
> at runtime, because whenever a peer sends a message to the container,
> flatpak-dbus-proxy gives the container SEE access to that peer.)

This could be added as (flags=SEE,bus_name=NAME, object_path="/", interface="") (optionally adding OBJECT_PATH_IS_SUBTREE, but the object path is ignored for SEE anyway).

We could emulate this by having a data structure (hash table?) containing all the unique names (or equivalently, connections) that the container instance is allowed to SEE, and ensuring that every peer that sent a message to the container instance is added to that data structure. This is probably better than synthesizing "full-fat" Allow rules for them.
Comment 6 Simon McVittie 2017-10-26 18:12:12 UTC
(In reply to Simon McVittie from comment #4)
> Unexpected replies are always blocked, in both directions. Expected replies
> are always allowed, in both directions.

I think having the Allow named-parameter at all should implicitly allow this, then we don't need a representation of it.

> Sending broadcast signals is always allowed.

I'm a little hesitant about *always* allowing this, so I would be tempted to make it conditional. If we create a new SEND_BROADCASTS flag, then Flatpak could add (flags=SEND_BROADCASTS|OBJECT_PATH_IS_SUBTREE, bus_name="", object_path="/", interface=""). bus_name would be ignored for SEND_BROADCASTS, unless we wanted to have some condition like "if it's non-empty, the sender must own it".

> Peer-to-peer messages to the dbus-daemon are always allowed. In practice,
> the dbus-daemon only handles Peer messages here (anything else has to have
> o.fd.DBus as destination).

Replying to a Peer message is no more expensive than sending back an error, so I think we just allow this implicitly. If we want to do something to anonymize GetMachineId(), I think we should probably virtualize it (a HMAC where the real machine ID is the secret and the app ID is the message?) instead.

> Directed messages (method calls or unicast signals) to connections other
> than o.fd.DBus for which the container has TALK or OWN access are always
> allowed.

Comment #5 implements this.

> Directed method calls (only - no unicast signals) sent to connections for
> which we have FILTERED access are allowed if they match the filter.

Comment #5 implements this.

> Introspecting o.fd.DBus is always allowed. So are Hello, RemoveMatch and
> GetId.

I think these are OK to always allow.

> Special case for AddMatch: it is allowed, if it isn't eavesdropping.

dbus-daemon already treats eavesdropping as privileged, so that's straightforward.

> UpdateActivationEnvironment and BecomeMonitor are never allowed.

These are currently privileged, although see Bug #103458.

> RequestName, ReleaseName and ListQueuedOwners are allowed iff we have OWN
> access to the name.

These will need special cases in their implementations, but we can encapsulate that in the OWN_BUS_NAME flag.

> Special case for ListActivatableNames, ListNames, NameHasOwner,
> GetNameOwner, GetConnectionWhatever: names to which we don't have SEE access
> are invisible.

These will need special cases in their implementations, which we can encapsulate in the SEE flag and the data structure in Comment #5.

> Special case for StartServiceByName: it requires TALK access (in particular
> FILTERED access is not enough).

In Comment #5 I suggested a new ACTIVATE flag for this.

> Special case for NameOwnerChanged: only names to which we have >= SEE access
> are listed, but --sloppy-names is equivalent to having SEE access to every
> unique name. If a client has >= SEE access to a well-known name, and
> receives NameOwnerChanged for that well-known name, then that client (but
> not other clients in the same proxy?) is granted SEE access to the old and
> new owners as well.

We'd need a special case in the implementation. Comment #5 provides this.

> Other dbus-daemon methods and interfaces are blocked.

Because this *is* dbus-daemon, we can use METHOD_FLAG_PRIVILEGED, METHOD_FLAG_NO_CONTAINERS, or special-cases in the implementation.

> Receiving a method call or a unicast signal from a peer

I think it would be reasonable to say that method calls and unicast signals from the outside are always controlled by the sender, and container instances always allow receiving them.

> Receiving a method call or a unicast signal from a peer grants the recipient
> (but no other client of the same proxy) SEE access to that peer. I think we
> can relax that to giving everything in the same container instance SEE
> access to that peer, because there is no security boundary within a
> container instance.

The data structure suggested in Comment #5 can provide this.

> Receiving broadcast signals from outside the container instance checks for
> TALK access to the sender.

Comment #5 proposes RECEIVE_BROADCASTS.

> flatpak-dbus-proxy users can always TALK to their own unique name. Because
> OWN implies TALK, they can also TALK to anything in the same container
> instance, as long as communication was initiated using the well-known name.
> I think it would be reasonable to relax this to say that talking to other
> unique names in the same container instance is fine, because there is no
> security boundary within a container instance.

I think this can just be implicit.
Comment 7 Simon McVittie 2017-10-27 13:55:29 UTC
(In reply to Simon McVittie from comment #5)
> (In reply to Simon McVittie from comment #3)
> > * flatpak-dbus-proxy --filter --sloppy-names 
> 
> The equivalent of --sloppy-names would need to be a new flag (either an
> Allow rule with the flag or a separate named parameter),
> SEE_ALL_UNIQUE_NAMES or SeeAllUniqueNames.

It's a little less strict than flatpak-dbus-proxy, but I think it would be better to be a bit more consistent, and say that this flag also makes ListNames, NameHasOwner and GetNameOwner see all unique names. (In principle ListActivatableNames is also affected, but unique names aren't activatable.)
Comment 8 Simon McVittie 2017-10-27 15:29:04 UTC
(In reply to Simon McVittie from comment #5)
> The equivalent of --talk=NAME is
> (flags=SEE|CALL_METHODS|SEND_UNICAST_SIGNALS|RECEIVE_BROADCASTS|SEND_UNIX_FDS
> |RECEIVE_UNIX_FDS|OBJECT_PATH_IS_SUBTREE|ACTIVATE, bus_name=NAME,
> object_path=/, interface=""). (New flags needed here since Comment #0:
> SEND_UNICAST_SIGNALS, ACTIVATE.)

Alternatively, ACTIVATE could be implied by CALL_METHODS rather than existing as a separate flag, so you can StartServiceByName(foo) if there exists any method that you can call on foo. (Justification: if you can call any method on it, then you could autostart it, so there is no point in not letting you activate it explicitly.)

The reason I'm separating SEND_UNICAST_SIGNALS from sending broadcasts is that unicast signals have been used to exploit bugs in D-Bus users in the past (CVE-2013-0292 is a particularly egregious example).

The reason I'm separating SEND_UNICAST_SIGNALS from CALL_METHODS is that the object path, interface and (if present) member name have different meanings: for a signal they're facts about the sender, but for a method call they're facts about the recipient.

> We need a new flag INTERFACE_IS_REALLY_METHOD

Or INTERFACE_IS_MEMBER to generalize it to signals, although flatpak-dbus-proxy can't actually do that.

Flatpak uses com.example.Interface.Member or com.example.Interface.*, but I think I prefer to emphasize the common use-case (access at a per-interface granularity), which works well if you break up your interfaces by intended audience/level of privilege.

We could call the struct member interface_or_member and the flag SPECIFIC_MEMBER to make the naming less weird.
Comment 9 Simon McVittie 2017-10-27 16:00:11 UTC
(In reply to Simon McVittie from comment #5)
> This could be added as (flags=SEE,bus_name=NAME, object_path="/",
> interface="") (optionally adding OBJECT_PATH_IS_SUBTREE, but the object path
> is ignored for SEE anyway).

Justification for having SEE as an explicit flag: if we don't, then we can't use (RECEIVE_UNIX_FDS|OBJECT_PATH_IS_SUBTREE, "", "/", "") to mean "trusted processes can fd-pass when they call this container's methods" 

I want to lock down fd-passing by default so that we have the option of doing so: because the access control model is purely additive, we should only allow things by default if we're absolutely sure they're always fine. We have an "escape hatch" to tighten it up by adding new named parameters, but let's try not to need that. While fd-passing is extremely useful, CVE-2014-3637, CVE-2014-3635, CVE-2014-3636, CVE-2014-3532, CVE-2014-3533 are not a particularly inspiring track record.

I don't think Flatpak is designed to prevent denial of service by a contained app, in which case only CVE-2014-3635 would have been considered to be a problem; so it's probably the right trade-off for Flatpak to opt in to sending and receiving fds. But, as with the restriction on sending broadcasts, let's not rule out container technologies that might want to be more strict than Flatpak.
Comment 10 Simon McVittie 2017-10-27 16:51:05 UTC
(In reply to Philip Withnall from comment #2)
> (In reply to Simon McVittie from comment #0)
> >           [TODO: Does it allow receiving unicast signals from connections
> >           that are not in a container?]
> 
> Yes, that’s used by a few system service implementations, right?

We need to make this possible, sure; my question was whether it's always allowed, or whether it's something we opt-in to with a flag. I think I've come down on the side of always allowing it.
Comment 11 Simon McVittie 2017-10-27 16:59:15 UTC
(In reply to Simon McVittie from comment #4)
> Special case for StartServiceByName: it requires TALK access (in particular
> FILTERED access is not enough).

I think this is more strict than it needs to be. It is sufficient that there exists at least one method call that we are allowed to send to the service (because if so, we could call it and rely on auto-starting).
Comment 12 James Henstridge 2017-12-22 11:14:32 UTC
This is based on some of the discussion on the Snapcraft forum here:

https://forum.snapcraft.io/t/interacting-with-dbus-daemons-app-container-feature/3245

If snapd continues to rely on AppArmor for mediating dbus messages, I think we can work with this as is by installing a permissive ACL on the container socket.

To make this filtering system useful for snapd, we'd like a way to replace the base ACL for a container after passing the socket to dbus-daemon.  The GrantAccess/RevokeAccess methods don't look like they fit the bill: they seem to be designed to augment the ACLs of all containers to talk to the calling dbus service.

Instead, we probably want something like:

  SetContainerAccess(s: container_id,
                     a(usos): rules)

... that replaces the base ACL of a container: both for existing connections accepted by the container socket and for new ones.

As for the expressiveness of the access control rules here are a few wishlist items:

1. It would be nice to be able to grant the access to services provided by another container, rather than working backwards from the names the other container is allowed to own.

This probably can't be implemented in terms of the dbus-daemon allocated container IDs, since that adds ordering constraints.  Rather, matching (container_type, app_identifier) would be useful.

2. With some legacy interfaces, there is a mix of safe and unsafe methods attached to a single interface.  While it would be nice if these were rewritten to separate those methods onto different interfaces, it would be nice to be able to be able to grant access to a subset of methods.

3. This isn't needed by snapd, but would it make sense to special case the properties interface?  Perhaps add a GET_PROPERTIES flag that will allow calls to o.fd.DBus.Properties.Get() and GetAll() with the given interface as first argument, and a SET_PROPERTIES flag that does the same for Set()?

Without something like this, you've only got the choice to grant read/write access to all properties for a particular object or no access at all.
Comment 13 Simon McVittie 2018-01-02 12:43:10 UTC
(In reply to James Henstridge from comment #12)
> Instead, we probably want something like:
> 
>   SetContainerAccess(s: container_id,
>                      a(usos): rules)
> 
> ... that replaces the base ACL of a container: both for existing connections
> accepted by the container socket and for new ones.

It seems like this should replace the base rules that were passed in the Allow named parameter (Comment #0), but should not replace rules added by individual services like dconf to open up more access to themselves (GrantAccess, RevokeAccess), because otherwise those services need to negotiate with the container manager. Does that seem reasonable?

We'll need some terminology to identify those rules to make it clear what's going on.

> 1. It would be nice to be able to grant the access to services provided by
> another container, rather than working backwards from the names the other
> container is allowed to own.
> 
> This probably can't be implemented in terms of the dbus-daemon allocated
> container IDs, since that adds ordering constraints.  Rather, matching
> (container_type, app_identifier) would be useful.

(Terminology: as far as dbus-daemon is concerned, the *container manager* is whoever calls AddServer().)

The (container type, app ID) pair is more problematic because on the system bus, in general, we have no way to know how trustworthy those fields are: we can only know that uid 1234 *claims* the server is for the pair (Flatpak, Firefox) or whatever, and in principle uid 1234 could have been lying to us. See Bug #103457. I know Snap uses high-privilege processes, but that isn't the case for all container technologies: the Flatpak process that launches a sandboxed app is no more privileged than the same user's gnome-session or systemd --user or whatever.

For Snap, perhaps the container manager process is always trusted/privileged anyway; presumably it needs CAP_MAC_ADMIN to deal with AppArmor profiles? So perhaps there can be a way to prove to our satisfaction that a particular container manager is trustworthy (perhaps "it has uid 0 or the uid of the dbus-daemon"), and then be willing to use the metadata that was attached to servers by that container manager in access-control decisions, even though we are not willing to do the same for other servers' metadata? But that does seem hard to document.

Or to support the special case of Snap with AppArmor, perhaps we could use the security label instead, with a SECURITY_LABEL_INSTEAD_OF_BUS_NAME flag? That's reliable and globally unique.

> 2. With some legacy interfaces, there is a mix of safe and unsafe methods
> attached to a single interface.  While it would be nice if these were
> rewritten to separate those methods onto different interfaces, it would be
> nice to be able to be able to grant access to a subset of methods.

That's easy to add, and is already required for feature parity with flatpak-dbus-proxy (Comment #8).

> 3. This isn't needed by snapd, but would it make sense to special case the
> properties interface?  Perhaps add a GET_PROPERTIES flag that will allow
> calls to o.fd.DBus.Properties.Get() and GetAll() with the given interface as
> first argument, and a SET_PROPERTIES flag that does the same for Set()?
> 
> Without something like this, you've only got the choice to grant read/write
> access to all properties for a particular object or no access at all.

If someone needs this, it would be easy to add. I'm a little reluctant to add functionality speculatively, but this might make sense anyway.

I agree that o.fd.DBus.Properties.{Get,GetAll,Set} behave more like members of the interface that is their first argument than like an ordinary interface.

Two possibilities for this:

* (CALL_METHODS, bus name, object path, "o.fd.DBus.Properties") implies (GET_PROPERTIES|SET_PROPERTIES, bus name, object path, ""), so a call to Get("some.iface", "Prop") is allowed if you are allowed to call Get() *or* you are allowed to get Prop

* CALL_METHODS for the Properties interface and {GET,SET}_PROPERTIES for the interface you are interested in are checked individually, so  a call to Get("some.iface", "Prop") is allowed if you are allowed to call Get() *and* you are allowed to get Prop
Comment 14 Simon McVittie 2018-01-02 12:46:46 UTC
(In reply to Simon McVittie from comment #13)
> I agree that o.fd.DBus.Properties.{Get,GetAll,Set} behave more like members
> of the interface that is their first argument than like an ordinary
> interface.

Note that PropertiesChanged can contain the new values of properties, and I'm not sure whether I really want to be special-casing the PropertiesChanged signal so only peers with GET_PROPERTIES for the relevant interface can see it, even if they have RECEIVE_BROADCASTS for the Properties interface (or, more likely, all interfaces).

Perhaps GET_PROPERTIES is unnecessary, and we only really need finer-grained control over *setting* properties?
Comment 15 James Henstridge 2018-01-02 13:09:30 UTC
Wouldn't filtering PropertiesChanged at the granularity of interfaces be just as easy as the Get/Set method calls?  It's just doing a string match on the first argument in the message.

One other place where property values can leak out is via ObjectManager.  But I'm not sure if that one is worth trying to address: if you grant ObjectManager access to a confined client, you're implicitly exposing a lot of other information about objects at sub-paths.
Comment 16 James Henstridge 2018-01-04 08:54:00 UTC
One other question about this feature is whether it is intended to be usable as mandatory access control or not.

If we take the passed to AddServer() alone, it looks like a pretty standard MAC system.  But letting uncontained services grant additional access to contained processes, it's looking a bit more like discretionary access control.

For cases where you actually want MAC, would it make sense to specify that a particular container shouldn't inherit any access rules from other services?

Or perhaps require something to appear in the base rules before the service rules are activated?
Comment 17 Simon McVittie 2018-01-04 14:00:50 UTC
(In reply to James Henstridge from comment #15)
> Wouldn't filtering PropertiesChanged at the granularity of interfaces be
> just as easy as the Get/Set method calls?  It's just doing a string match on
> the first argument in the message.

Yes ish, but preventing calling methods (caller gets an error back) is less nasty than preventing receiving signals (subscriber will never know what they missed).

> One other place where property values can leak out is via ObjectManager. 
> But I'm not sure if that one is worth trying to address: if you grant
> ObjectManager access to a confined client, you're implicitly exposing a lot
> of other information about objects at sub-paths.

Sure. For efficiency reasons alone, ObjectManager is only appropriate if "most" clients will want to know about "most" child objects. It would seem reasonable to say that access to an ObjectManager-based API is all-or-nothing.
Comment 18 Simon McVittie 2018-01-04 14:26:45 UTC
(In reply to James Henstridge from comment #16)
> One other question about this feature is whether it is intended to be usable
> as mandatory access control or not.

Sort of. Using AppArmor as an analogy, the security model for these containers/sandboxes as currently designed is as though everything outside the container is unconfined and has CAP_MAC_ADMIN. It's asymmetric: we want to protect the "real system" from the sandboxed processes, but protecting the sandboxed processes from the "real system" isn't really in scope.

For the session bus, I don't think anything else makes sense - things like gnome-keyring rely on being able to call UpdateActivationEnvironment() (which is arbitrary code execution because you can inject LD_PRELOAD into a session service), and in general non-sandboxed apps can scribble over each other's configuration files/dconf subtrees/etc. and get arbitrary code execution that way anyway.

For the system bus it's less obvious what's right.

> If we take the passed to AddServer() alone, it looks like a pretty standard
> MAC system.  But letting uncontained services grant additional access to
> contained processes, it's looking a bit more like discretionary access
> control.

The initial use case for GrantAccess() was for dconf:
https://bugs.freedesktop.org/show_bug.cgi?id=100344#c1
https://bugs.freedesktop.org/show_bug.cgi?id=100344#c3

The idea was that apps that use dconf would have permission to send method calls to a shared rendezvous object path to say "hello, I'm here", and in response, dconf would look at their Flatpak/Snap metadata and grant them access to appropriate parts of the configuration tree. This needs to be done dbus-daemon-side because that's the only reasonable way to give them permission to receive a runtime-varying subset of broadcasts.

> For cases where you actually want MAC, would it make sense to specify that a
> particular container shouldn't inherit any access rules from other services?
> 
> Or perhaps require something to appear in the base rules before the service
> rules are activated?

I think the second makes more sense: we could have a rule that effectively says "allow whatever communication with ca.desrt.dconf gets granted at runtime by ca.desrt.dconf", and all dconf-using apps would eventually have that rule?

(At the moment dconf is just not sandbox-friendly, either with flatpak-dbus-proxy or AppArmor or anything else - you can either have full read/write access or no access at all. The GrantAccess() feature is infrastructure work towards fixing that.)

AppArmor separates send and receive rules, but not every sent message is a write and not every received message is a read (method calls are not equal to signals, or for that matter other method calls), so I'm not convinced that distinction is a particularly useful one. When I was working on an automotive OS that used AppArmor, in practice we wrote a lot of "dbus (send,receive)" rules, because nearly every use of D-Bus turns out to be bidirectional.
Comment 19 Simon McVittie 2018-01-04 14:36:45 UTC
(In reply to Simon McVittie from comment #13)
> * (CALL_METHODS, bus name, object path, "o.fd.DBus.Properties") implies
> (GET_PROPERTIES|SET_PROPERTIES, bus name, object path, ""), so a call to
> Get("some.iface", "Prop") is allowed if you are allowed to call Get() *or*
> you are allowed to get Prop

I think this perhaps makes more sense than the other possibility in Comment #13, because it's the only thing that could work without breaking compatibility if we wanted to add CALL_METHODS immediately but GET_PROPERTIES, SET_PROPERTIES later on, so it'll be more consistent with any other special cases that we might need to add later. It would also avoid needing a special case for ObjectManager.

Pseudocode:

    if (message is method call) {
        if (message is Get() or GetAll()) {
           if (we allow GET_PROPERTIES, matching recipient details) {
               return ALLOW;
           }
        }

        if (message is Set()) {
           if (we allow SET_PROPERTIES, matching recipient details) {
               return ALLOW;
           }
        }

        if (we allow CALL_METHODS, matching recipient details) {
            return ALLOW;
        }

        maybe log a message;
        return DENY;
    }

(This only works because I'm deliberately not allowing negative rules, which is a lesson learned from dbus-daemon's XML ACL stuff - applying an ordered list of allow and deny rules can have much more surprising results than starting with deny-everything and applying an unordered list of allow rules.)
Comment 20 Simon McVittie 2018-03-21 12:35:13 UTC
Progress: I've started working on this, but it's a big task. So far I have ~ 1750 lines of a framework for unit-testing it, together with a few hundred lines of the most basic possible implementation (if present, Allow must be empty; if so, all the restrictions are applied). The design seems plausible, but the implementation and tests don't actually compile yet.

After I get that to compile and pass tests, the next steps will be to add increasingly many Allow rules, with tests that assert that they work.
Comment 21 GitLab Migration User 2018-10-12 21:31:43 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/185.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.