Created attachment 126648 [details] [review]
Proposed patch against 1.10.8

(In reply to Slava Monich from comment #0)
> Is there a reason why effective group id of the process is ignored when
> checking client credentials against the policy?

Background, so we have our terminology straight:

In Unix, each process has a single *effective user ID*, a single *effective primary group ID*, and zero or more *supplementary group IDs*. A process is traditionally considered to have the privileges associated with a group if either its effective group ID is that group, or its supplementary group IDs include that group.

In the system user database (normally /etc/passwd and /etc/group), each user ID is also associated with one primary group ID and zero or more supplementary group IDs. When a process changes uid from root to that user ID, it *usually* gets those group IDs. However, this is not guaranteed, and the code running as root is free to do something else instead (for example via pam_group).

(End of background.)

AF_UNIX sockets tell us the effective user ID and the effective primary group ID of the other end of the socket. Unfortunately, they do not tell us the supplementary group IDs, and there does not appear to be any way we can find them out without introducing race conditions that would let a malicious process claim the privileges of groups that it does not actually have.

At the moment, dbus-daemon resolves this by looking the user up in the system user database, and assuming that the calling process has the privileges of the user's primary group and the user's supplementary groups (but not any extra groups that the calling process might have been granted, for example via a setgid binary or pam_group). This is not ideal, and pam_group users complain about it every few years, but at least it's consistent.

Your proposed change seems to be to have dbus-daemon assume that the calling process has the privileges of its effective primary group ID (but not its effective supplementary groups, because we can't determine those safely!) in addition to the user's primary group and the user's supplementary groups.

That seems a lot harder to explain than the current behaviour: instead of group-based permissions being purely a property of the uid, your proposed change makes them partially determined by the uid's entry in the system user database, and partially by the process state. Security mechanisms that are hard to explain make me uncomfortable.

In some OSs, it might be possible to send messages that have an unforgeable out-of-band credentials struct proving that the process had the privileges of a particular group, either the effective primary group ID or an effective supplementary group ID, at the time of sending *this specific message*. If we solve "the pam_group problem" at all, I suspect that that might be a better approach to take. Callers might be expected to use pseudocode like this:

   m = dbus_message_new_method_call (...);
   ... add arguments ...
   dbus_message_prove_unix_group_id (m, my_group);
   ... send message and block for reply ...

However, this would require new protocol and API development.

Thanks for the response. Here is some background from my side. In Sailfish OS some user processes are started with effective gid 'privileged' (settings app, voicecall management and such). The user itself is not part of that group, which allows those selected processes to access more stuff than a regular app. There are essentially 3 levels of access: root, user:privileged and user. We would like to extend this sort of security model to D-Bus APIs as well.

(In reply to Slava Monich from comment #3)
> In Sailfish
> OS some user processes are started with effective gid 'privileged' (settings
> app, voicecall management and such). The user itself is not part of that
> group, which allows those selected processes to access more stuff than a
> regular app.

Are those processes launched from setgid binaries, or do they get this magic group-ID some other way?

Without having looked at their source code or anything, I think it's overwhelmingly likely that (euid "user", egid "user") can execute arbitrary code as (euid "user", egid "privileged") somehow. Common Unix frameworks are not designed to protect a uid from themselves, and the session bus is not designed to be a security boundary.

Without really thinking about it very long at all, here are lots of ways to pwn those processes:

* ptrace (although setgid probably prevents that)
* reading dotfiles that can be written by "user" and trusting their contents
* inheriting environment variables (LD_PRELOAD, GTK_MODULES, PYTHONPATH, ...)
  and trusting their contents
* trusting the "dbus-daemon --session" instance to give the right answers
  to GetConnectionCredentials(), unless it is *also* privileged
* trusting the "dbus-daemon --session" instance to apply <policy>,
  unless it is *also* privileged
* trusting other processes' unprivileged D-Bus APIs to give the right
  answers
* inheriting DBUS_SESSION_BUS_ADDRESS from the environment and trusting
  that it points to the expected "dbus-daemon --session" that we can trust,
  and not to a malicious proxy that says what we want to hear in order
  to achieve privilege escalation

Some more explanation of that last one: in the D-Bus protocol, clients authenticate themselves to the server, but the server does not authenticate itself to clients. Instead, the server's address is assumed to be obtained from a trustworthy source. If you have obtained the server's address from something that might not be trustworthy, you can't win: it could be an attacker-controlled server.

In any case, if you are using the session bus as a security boundary, you will also need to rewrite its configuration (session.conf) to deny receiving eavesdropped messages, and deny access to BecomeMonitor(), like the system bus does; if you are interested in preventing or mitigating denial of service attacks, you will also have to apply arbitrary limits that are less than "all of memory", like the system bus does. This is really not what it's designed for.

Alternatively, you could turn the situation around: run unprivileged processes in a sandbox, and run the privileged and trusted processes (including dbus-daemon) outside the sandbox. Flatpak implements that by having the sandboxed processes run in a namespace that cannot see the dbus-daemon, and putting a filtering proxy in the way; Snappy implements that by using an LSM (AppArmor) to deny accesses that would be problematic, and modifying the dbus-daemon to "mediate" (filter) messages using LSM rules. Neither makes much attempt to prevent or mitigate DoS. Both are valid approaches, although Flatpak's namespace-based approach seems more likely to be feasible to make secure in practice.

(In reply to Simon McVittie from comment #2)
> Your proposed change seems to be to have dbus-daemon assume that the calling
> process has the privileges of its effective primary group ID (but not its
> effective supplementary groups, because we can't determine those safely!) in
> addition to the user's primary group and the user's supplementary groups.
> 
> That seems a lot harder to explain than the current behaviour: instead of
> group-based permissions being purely a property of the uid, your proposed
> change makes them partially determined by the uid's entry in the system user
> database, and partially by the process state. Security mechanisms that are
> hard to explain make me uncomfortable.

If your proposed mechanism used a new <policy process_primary_group="privileged"> (or something) instead, that would at least make it clearer that this is about the primary group of the process, unlike the existing group="whatever" which is about any the user's groups in the system group database.

However, what we've been saying for the last few years is:

* session/user services shouldn't try to be a security boundary
* system services should use polkit instead of the (horrible) XML policy
  language, for more or less everything except <allow own>

To make this information available to services (including polkit), it would have to go in the result of GetConnectionCredentials(), perhaps as a new "UnixProcessPrimaryGroupID" key. <https://dbus.freedesktop.org/doc/dbus-specification.html#bus-messages-get-connection-credentials>

(In reply to Simon McVittie from comment #2)
> AF_UNIX sockets tell us the effective user ID and the effective primary
> group ID of the other end of the socket. Unfortunately, they do not tell us
> the supplementary group IDs, and there does not appear to be any way we can
> find them out without introducing race conditions that would let a malicious
> process claim the privileges of groups that it does not actually have.

As of Linux 4.13, this is no longer true. Bug #103737 tracks the addition of a group list from SO_PEERGROUPS, which is exactly the mechanism we wanted.

After that feature request is implemented, the client's effective primary group ID and its complete auxiliary group list will be the group list used by dbus-daemon (but only if dbus-daemon was built on a system with Linux 4.13+ kernel headers available, *and* is used on a system with a Linux 4.13+ kernel).

If non-Linux OSs develop an equivalent of SO_PEERGROUPS, please open a separate feature request blocked by Bug #103737 (the infrastructure to deal with groups and the Linux-specific implementation are both part of that bug) to plumb the other OS's equivalent of SO_PEERGROUPS into add_groups_to_credentials().

> Your proposed change seems to be to have dbus-daemon assume that the calling
> process has the privileges of its effective primary group ID (but not its
> effective supplementary groups, because we can't determine those safely!) in
> addition to the user's primary group and the user's supplementary groups.
> 
> That seems a lot harder to explain than the current behaviour

Now that SO_PEERGROUPS exists, I think that part is WONTFIX: it provides less information, and is harder to document and use.

That means the only thing remaining in scope is effectively Bug #103737, so I'm marking this as a duplicate.

*** This bug has been marked as a duplicate of bug 103737 ***