Comment on attachment 74611 [details] [review]
Allows the session bus address to be visible in suid/sgid programs

Review of attachment 74611 [details] [review]:
-----------------------------------------------------------------

No, this patch undermines the mitigation of CVE-2012-3524 (Bug #52202) and can lead to arbitrary code execution under the privileged uid/gid by the less-privileged caller:

* if DBUS_SESSION_BUS_ADDRESS=autolaunch: then libdbus will execute dbus-launch, which is not intended to be privileged, with the host process' EUID and EGID

* if DBUS_SESSION_BUS_ADDRESS=unixexec:path=/bin/rm%20-rf%20/ (for instance) then the command will be executed with the host process' privileges, including EUID and EGID

The fundamental problem here is that in a setuid program, the environment is supplied by the caller; so if we trust the environment, and use it to do things with our escalated privileges, then the caller can subvert us. Setuid programs should filter the environment through a whitelist (it's the only safe thing to do), so this change shouldn't be effective anyway; but many setuid programs don't do that, which is why libdbus has to try to protect them from themselves.

Here are some alternatives to having a setuid program that tries to join the session bus:

* have a minimal setuid "helper" which verifies that it is only escalating privileges in the allowed way, then does the privileged action; make the unprivileged process in the user's session run it with command-line arguments or something to tell it what to do

* have a service on the system bus which does the privileged action, for instance using PolicyKit to determine whether it should allow the unprivileged user to take that action; this doesn't need to be setuid, it can just have User= in its .service file (udisks takes this approach)

(In reply to comment #1)
> Comment on attachment 74611 [details] [review] [review]
> Allows the session bus address to be visible in suid/sgid programs
> 
> Review of attachment 74611 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> No, this patch undermines the mitigation of CVE-2012-3524 (Bug #52202) and
> can lead to arbitrary code execution under the privileged uid/gid by the
> less-privileged caller:
> 
> * if DBUS_SESSION_BUS_ADDRESS=autolaunch: then libdbus will execute
> dbus-launch, which is not intended to be privileged, with the host process'
> EUID and EGID

  Excuse me, as far as I know the autolaunch by suid/sgid process is explicitly blocked at _dbus_get_autolaunch_address():

--- dbus / dbus-sysdeps-unix.c ---
if (_dbus_check_setuid ())
{
    dbus_set_error_const (error, DBUS_ERROR_NOT_SUPPORTED,
                          "Unable to autolaunch when setuid");
    return FALSE;
}
---

Created attachment 74799 [details] [review]
Remove the extra "saved set-UID/GID" check

  Let me try an other approach, please. While having some privileged EUID and/or EGID values may lead to the problems mentioned above, it isn't true for the "saved set-user-ID" (suid) and "saved set-group-ID" (sgid). These values are not inherited by the new programs run with exec*() system calls. If there would be no check for suid/sgid values in libdbus, a privileged program would have to simply reset the euid/egid values to the real IDs prior to get a connection to the session bus and get full-feature D-Bus communication.
  A simple patch is attached.

(In reply to comment #4)
>   Let me try an other approach, please. While having some privileged EUID
> and/or EGID values may lead to the problems mentioned above, it isn't true
> for the "saved set-user-ID" (suid) and "saved set-group-ID" (sgid).

Sorry, having had a usable root exploit facilitated by libdbus in the (recent) past[1], I'm not really willing to compromise on this. Perhaps there's nothing in libdbus *right now* that can be subverted via environment variables without first calling exec(), but I'm not comfortable with assuming that that will always be the case.

(If you can execute arbitrary code without an exec(), then malicious code could just restore the saved UID/GID, e.g. with setresuid(), and you have a vulnerability again.)

Separate your privileged and unprivileged code, please - Comment #1 describes the state-of-the-art. The first of the approaches I suggested is used by D-Bus' dbus-daemon-launch-helper and PolicyKit's pkexec - you will notice that both of those clean their environment before proceeding.

The second of the approaches I suggested is used by just about every D-Bus system service that runs as root, including udisks, upower, NetworkManager, Avahi, and systemd's various secondary daemons like systemd-timedated.

Or, if you want to drop privilege completely, setresuid(ruid, ruid, ruid) can reset all three UIDs to the real UID, and is somewhat widely available (Linux, some BSDs including FreeBSD, and HP-UX, according to the Linux man page for it).

---

[1] CVE-2012-3524 (Bug #52202). D-Bus maintainer consensus at the time was that setuid executables must not use libdbus without first cleaning their environment, but that libdbus must prevent the attack anyway, because in practice, many setuid executables do not clean their environments (and most notably, su, sudo, etc. call into arbitrary libraries via PAM, potentially including libdbus, without clearing the attacker-controlled environment).

  Thanks, Simon. Just thought you might advice me something if I let you know the root problem that I'm trying to solve. Surprisingly, I just try to use the E17 (Enlightenment) DE. :) Be more precise, to adapt it for the ALT Linux/Sisyphus distributions. And I got to the place, where two security restrictions faced each other:

-rwx--s--x 1 root chkpwd 1731136 Feb 13 21:12 /usr/bin/enlightenment

The "chkpwd" group is necessary here for success of the password checking procedure (pam_tcb) in the integrated desktop locker. While the other DE uses separate executables for screensaver/loker, E17 uses modules that run inside the single process. And, as you may guess, that process uses D-Bus, too.

  I'm trying to find a minimal possible change that allows accessing pam_tcb and (already run) session bus from the same process.

(In reply to comment #6)
> And I got to the place, where two security
> restrictions faced each other:
> 
> -rwx--s--x 1 root chkpwd 1731136 Feb 13 21:12 /usr/bin/enlightenment
> 
> The "chkpwd" group is necessary here for success of the password checking
> procedure (pam_tcb) in the integrated desktop locker. While the other DE
> uses separate executables for screensaver/loker, E17 uses modules that run
> inside the single process. And, as you may guess, that process uses D-Bus,
> too.

This arrangement appears to mean that all 1.7MB of Enlightenment, plus an unknown number of libraries and modules that it links, effectively get "chkpwd" privileges. I don't think that makes sense: if there is any opportunity for arbitrary code execution (either via an exploitable bug, or deliberate plugin/module hook points) anywhere in that stack, then the user who's running the E17 session can run arbitrary code with chkpwd privileges, which would mean there's no security benefit over having that user just be a member of chkpwd themselves!

Now, I don't know exactly what chkpwd does: it might be like the shadow group on Debian, which has full read access to /etc/shadow, or it might only be able to do something relatively "safe" like running a please-check-my-password executable similar to pam_unix's unix_chkpwd. (On Debian, unix_chkpwd is setgid shadow; on some distributions it's setuid root instead.) If chkpwd membership is benign enough that the designers of Arch/Sabayon's security model consider it to be OK for desktop users to be able to escalate to chkpwd privileges, then... put desktop users in that group and get rid of the setuid, I suppose? Or, if it's a significant enough capability that it's necessary to lock it down, then it's a significant enough capability that an executable the size of E17 shouldn't have it. Pick one.

Having the desktop locker integrated into a large, general, user-configurable "shell" is a fine design - similarly, GNOME 3 in non-fallback mode integrates the screensaver into GNOME Shell rather than using an external gnome-screensaver - but if that's your design, then it shouldn't run with special privileges. Instead, it should call out to a secure, privileged service - either a tiny, short-lived setuid/setgid executable like unix_chkpwd, or a D-Bus system service daemon like the core part of gdm - which *does* run with special privileges, links a minimum number of libraries, and has been audited for security.

... GNOME 3 in non-fallback mode integrates the screensaver into GNOME Shell
rather than using an external gnome-screensaver...

  Uh, great! I will look at it. Thanks! :)

(In reply to comment #7)
> Now, I don't know exactly what chkpwd does:

It is a helper from tcb password shadowing scheme
(see http://docs.altlinux.org/manpages/tcb.5.html for details):

# stat -c '%A %8U %8G %8s %n' /usr/lib/chkpwd /usr/lib/chkpwd/tcb_chkpwd /etc/tcb /etc/tcb/gdm /etc/tcb/gdm/shadow
drwx--x---     root   chkpwd     4096 /usr/lib/chkpwd
-rwx--s--x     root   shadow    10408 /usr/lib/chkpwd/tcb_chkpwd
drwx--x---     root   shadow     4096 /etc/tcb
drwx--s---      gdm     auth     4096 /etc/tcb/gdm
-rw-r-----      gdm     auth       18 /etc/tcb/gdm/shadow

That is, a user's process needs permissions to execute /usr/lib/chkpwd/tcb_chkpwd just to check a user's own password.

(Sorry, not sure where "Arch/Sabayon" came from; I meant "ALT/Sisyphus".)

What security benefit do you believe you gain from restricting tcb_chkpwd to only be executable by certain programs?

I think the correct solution here is likely to be to audit and harden tcb_chkpwd, which as far as I can see is *already* a trusted system (in the sense of: a component whose failure can break your security model), to the extent that allowing it to be world-executable is safe; then the chkpwd group could just go away, and unauditable, multi-megabyte applications like E17 wouldn't have to be considered trusted.

(For comparison, PAM's unix_chkpwd, which appears to be closely related to tcb_chkpwd, is setgid shadow and executable by everyone.)

I'm not going to relax libdbus' setuid checks to work around this, and I advise distribution maintainers not to relax those checks either.