Possible resolutions include:

* Document that set*id applications must not call dbus_bus_get()
  or other affected functions (either at all, or without first sanitizing
  their environment). Check the set*id binaries in major distributions
  to make sure they don't.

* Add a new dbus_disable_environment() and document that set*id
  applications that link libdbus must call it before first use.
  Check the set*id binaries in major distributions to make sure that
  they either don't link libdbus, or do call dbus_disable_environment().

  (I don't like this option - there shouldn't be API for "make my
  application sane, please" that's off-by-default.)

* Similar to the above: stop obeying the environment in current APIs.
  In cases where obeying the environment is needed, add an "opt-in"
  way to do so.

  (This potentially breaks working, non-setuid applications that
  were relying on environment variables. In particular, many applications
  rely on DBUS_SESSION_BUS_ADDRESS.)

* Obey DBUS_SESSION_BUS_ADDRESS (because if you're set*id, you
  shouldn't be on the session bus anyway) but restrict
  DBUS_SYSTEM_BUS_ADDRESS (more rarely used, I suspect).

* Use glibc's __secure_getenv() or a new, documented equivalent, as
  suggested on the oss-sec thread. This pretends the environment is
  empty if the executable is detected to be privileged.
  (This only protects GNU systems; it may be how glibc avoids
  interpreting LD_LIBRARY_PATH for set*id binaries.)

* Reimplement something like __secure_getenv().
  (In practice, this probably only protects Linux systems
  with /proc mounted.)

* Use or reimplement __secure_getenv(). On non-GNU or non-Linux,
  pessimistically assume we are always privileged. (Probably breaks
  applications on non-GNU or non-Linux.)

(In reply to comment #1)
> * Document that set*id applications must not call dbus_bus_get()
>   or other affected functions (either at all, or without first sanitizing
>   their environment).

Maintainer consensus is that this is the case. We do not support use of libdbus in setuid binaries that do not sanitize their environment before their first call into libdbus.

Quoting from <http://seclists.org/oss-sec/2012/q3/116>:

On 10/07/12 14:09, Sebastian Krahmer wrote:
> We are going to add a libdbus hardening patch:
>
> https://bugzilla.novell.com/show_bug.cgi?id=697105
>
> This is because some suid binaries (Xorg and others) are linked
> against libdbus

The tl;dr: version if (e.g.) your Xorg binary still uses HAL and is also
setuid, ensure that it cleans its environment using a whitelist before
its first use of libdbus, libhal, any other non-trivial library, or exec().

In off-list discussion with the other D-Bus upstream maintainers,
consensus was that binaries with greater privileges than their parent
process (setuid or VFS capabilities) must not use non-trivial libraries
(e.g. libdbus, libhal, GIO or any scripting language), or run other
executables, without first sanitizing their environment using a
"whitelist" approach (start from a blank environment, set PATH etc. to
known-safe values, and copy only known-safe variables from the original
environment). Resetting the environment as close as possible to the
beginning of main() is wise.

In particular, we do not support use of libdbus in setuid binaries that
do not sanitize their environment before their first call into libdbus.
See
<http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/environment-variables.html>
for further advice.

Good examples:

* pkexec <http://cgit.freedesktop.org/polkit/tree/src/programs/pkexec.c>
has a small whitelist of environment variables to pass through
* The setuid wrapper generated by IkiWiki
<http://source.ikiwiki.branchable.com/?p=source.git;a=blob;f=IkiWiki/Wrapper.pm>
clears the environment and restores a small whitelist of CGI variables
before executing user code
* sudo appears to operate on a secondary environment internally, avoids
non-trivial libraries, and defaults to resetting the environment from a
whitelist

One maintainer specifically expressed opposition to "hardening" libdbus
if it will impede running non-setuid binaries unprivileged, under a fake
system bus, for regression testing. This rules out, for instance, using
secure_getenv() on systems with glibc >= 2.17 and always returning NULL
(i.e. assuming privilege) on other systems.

Using secure_getenv() on GNU systems and assuming *un*privileged
behaviour on non-GNU is not ruled out by that consideration, but leaves
non-GNU systems vulnerable. It also does not appear to address
executables with escalated privileges other than set*id (e.g. Linux VFS
capabilities). We believe that this makes it a poor substitute for
ensuring that setuid executables clear the environment properly, which
(in general) needs to be done anyway.

One very minor comment: for glibc, filesystem capabilities are treated in exactly the same way as setuid.  This is because the AT_SECURE ELF aux flag is set by the kernel, and the presence of that sets __libc_enable_secure to 1.

So using __secure_getenv() should be fine for projects that want to depend on it (but libdbus is not one of those).

So this bug says that we should document that setuid executables should "clear their environment", but the question arose - would we be willing to guarantee that libdbus only looked at environment variables starting with DBUS_ ?

I'm not sure...my worry is that we might say respect TMPDIR, and that could be used as an exploit vector.  Actually looking at the code, we do inspect TMPDIR, but only for nonce files...currently.

(In reply to comment #4)
> So this bug says that we should document that setuid executables should "clear
> their environment", but the question arose - would we be willing to guarantee
> that libdbus only looked at environment variables starting with DBUS_ ?

I would not be willing to make this guarantee, even if it was true. A whitelist is the only secure thing you can do in a set*id application, so we should not encourage people to use blacklists.

It isn't true anyway, because we (potentially) respect "standard" variables like $XDG_*, $HOME, $TMPDIR etc., because we link libraries we don't control which could respect arbitrary variables (libc), and because under some circumstances libdbus invokes dbus-launch which links more libraries we don't control (most prominently libx11).

Created attachment 67345 [details] [review]
Filter environment variables

After further investigation, it turns out there are multiple escalation vectors on different systems (via su/sudo + pam_systemd for example).  So I think it makes sense to harden libdbus here, even though these programs are still buggy.

su/sudo + pam_systemd is a kind of special case though...

Comment on attachment 67345 [details] [review]
Filter environment variables

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

None of this review blocks merging this patch and doing the advisory/release/release/release dance (in principle we currently support D-Bus 1.6, 1.4 and 1.2), but I would like to see a follow-up which improves on it and can also be picked up by distributions.

> libdbus never had an
> explicit policy about its use in setuid programs.
> I'm not sure whether we should advertise such support.

I'm pretty sure that we should not claim that libdbus (or any other non-trivial library) is safe to use in processes that are more privileged than their parent (setuid, filesystem capabilities, etc.) without first sanitizing all input from the parent - notably the environment and command-line.

However, people will inevitably get that wrong, and when they do, as you've argued, it's fine for us to mitigate that.

> document that setuid executables must clear their environment before using libdbus

I'd still like this to happen (Comment #2 has wording that could probably be adapted), but it's not critical-path.

(In reply to comment #6)
> it turns out there are multiple escalation
> vectors on different systems (via su/sudo + pam_systemd for example)

I think it's a bug whenever setuid executables (su/sudo) invoke non-trivial libraries with a caller-controlled environment, and PAM is an extremely non-trivial library.

sudo appears to have some logic for taking a copy of the caller-controlled environment, resetting its effective environment to safe values, doing its own stuff (including PAM), and restoring none, some or all of the caller-controlled environment (according to configuration) when it's about to exec the child process. I think this is the only safe way to combine setuid and PAM.

::: dbus/dbus-sysdeps-unix.c
@@ +4119,5 @@
> +dbus_bool_t
> +_dbus_check_setuid (void)
> +{
> +  /* TODO: get __libc_enable_secure exported from glibc.
> +   * See http://www.openwall.com/lists/owl-dev/2012/08/14/1

If we're doing this check at all, it does not seem desirable to behave as if non-setuid when we might have our normal uid/gid, but elevated filesystem capabilities.

A workaround for glibc does occur to me: if __secure_getenv ("HOME") (or secure_getenv ("HOME") in newer glibc) returns NULL, either we're setuid or "set-capabilities", or we genuinely do have no HOME. Behaving as if privileged in the absence of a HOME seems tolerable.

(Replace HOME with another standard environment variable in whose absence everything breaks anyway, if desired. PATH, maybe?)

On the other hand, it's possible that making _dbus_getenv() use secure_getenv() or __secure_getenv() (in preference to _dbus_check_setuid() + getenv()), as I suggest below, is already enough to protect us. If the exec*p family use secure_getenv for PATH, then running dbus-launch from PATH is harmless while (glibc thinks we're) setuid anyway.

Similarly, X11 autolaunching requires _dbus_getenv ("DISPLAY"), and writing "keyring" credentials requires _dbus_getenv ("HOME"), so if those (are made to) use secure_getenv, we're OK anyway.

launchd looks OK because it's a Mac OS X feature, and Mac OS X is FreeBSD-derived, so it hopefully has issetuidgid().

@@ +4137,5 @@
> +
> +  static dbus_bool_t check_setuid_initialised;
> +  static dbus_bool_t is_setuid;
> +
> +  if (_DBUS_UNLIKELY (!check_setuid_initialised))

This isn't necessarily thread-safe.

The thread initialization function (whichever of dbus_threads_init() or dbus_threads_init_default() is the real implementation in this particular branch - it swapped during 1.5) is required to be called before we start our second thread, so it should call _dbus_check_setuid() for its side-effect of initializing these static variables where required, with a comment.

::: dbus/dbus-sysdeps.c
@@ +190,1 @@
>    return getenv (varname);

This should use secure_getenv() (if it exists) or __secure_getenv() (if *that* exists) or _dbus_check_setuid()+getenv().

That would prevent environment variables from being interpreted if we're privileged but same-uid (e.g. filesystem capabilities). It can be reverted when glibc provides a better way to check for this.

(In reply to comment #7)
> Comment on attachment 67345 [details] [review] [review]
> Filter environment variables
> 
> Review of attachment 67345 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> None of this review blocks merging this patch and doing the
> advisory/release/release/release dance (in principle we currently support
> D-Bus 1.6, 1.4 and 1.2), but I would like to see a follow-up which improves
> on it and can also be picked up by distributions.

On it tomorrow.

> I'd still like this to happen (Comment #2 has wording that could probably be
> adapted), but it's not critical-path.

Any suggestions for *where* to document this?  Toplevel docs for dbus/dbus-bus.c?  

> sudo appears to have some logic for taking a copy of the caller-controlled
> environment, resetting its effective environment to safe values, doing its
> own stuff (including PAM), and restoring none, some or all of the
> caller-controlled environment (according to configuration) when it's about
> to exec the child process. I think this is the only safe way to combine
> setuid and PAM.

I may see if I can convince coreutils people to do something similar for su.
 
> If we're doing this check at all, it does not seem desirable to behave as if
> non-setuid when we might have our normal uid/gid, but elevated filesystem
> capabilities.
> 
> A workaround for glibc does occur to me: if __secure_getenv ("HOME") (or
> secure_getenv ("HOME") in newer glibc) returns NULL, either we're setuid or
> "set-capabilities", or we genuinely do have no HOME. Behaving as if
> privileged in the absence of a HOME seems tolerable.

I actually thought about testing whether getenv ("PATH") == __secure_getenv ("PATH") as the poor-man's issetugid() on Linux/glibc, but it's really a hack...

I'm not so sure about absence-of-HOME.  I mean, can't that happen for system services e.g.?

> On the other hand, it's possible that making _dbus_getenv() use
> secure_getenv() or __secure_getenv() (in preference to _dbus_check_setuid()
> + getenv()), as I suggest below, is already enough to protect us. If the
> exec*p family use secure_getenv for PATH,

Nope, from glibc/posix/execvpe.c:93:

      char *path = getenv ("PATH");

I think doing so would break some uses of sudo.

> This isn't necessarily thread-safe.

Will fix.
 
> This should use secure_getenv() (if it exists) or __secure_getenv() (if
> *that* exists) or _dbus_check_setuid()+getenv().

True, we might as well go the extra mile.  Will fix.

Created attachment 67803 [details] [review]
Use-__secure_getenv-if-available.patch

Created attachment 67804 [details] [review]
hardening-Ensure-_dbus_check_setuid-is-initialized-t.patch

Created attachment 67810 [details] [review]
activation-helper: Bypass _dbus_getenv filtering by using getenv

Unfortunately, there is one very well-known setuid program that links dbus, namely the activation helper. This patch breaks the ability of bus-activated services to find their starter bus, because DBUS_STARTER_ADDRESS gets cleared. (Specifically, the clear_environment function has logic to _dbus_getenv("DBUS_STARTER_ADDRESS"), clear everything else, and restore it, but if _dbus_getenv fails in setuid programs, the variable never gets restored.)

Attached is a patch that fixes this by using getenv directly in the activation helper, which should be fine since clear_environment does its own filtering.

That said, I'm not sure this patch is safe. Specifically, the DBUS_STARTER_ADDRESS=unixexec:... case worries me, in that it would reintroduce the vulnerability (the child of the activation helper, which is not setuid, would happily use that address). It's possible this is fine since the helper is only executable by the messagebus user?

I'm happy to talk this over with folks on IRC tomorrow; it's getting late for me so I'm not sure I want to make assertions about what is and isn't secure.

Comment on attachment 67803 [details] [review]
Use-__secure_getenv-if-available.patch

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

Looks good.

Comment on attachment 67804 [details] [review]
hardening-Ensure-_dbus_check_setuid-is-initialized-t.patch

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

::: dbus/dbus-sysdeps-pthread.c
@@ +276,4 @@
>  _dbus_threads_init_platform_specific (void)
>  {
>    check_monotonic_clock ();
> +  (void) _dbus_check_setuid ();

I think this deserves a comment above:

/* Not necessarily thread-safe, so we call it for its
 * side-effects before allowing threaded use. */

Other than that, looks good.

Comment on attachment 67803 [details] [review]
Use-__secure_getenv-if-available.patch

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

::: dbus/dbus-sysdeps.c
@@ +184,5 @@
>  {  
> +#ifdef HAVE___SECURE_GETENV
> +  return __secure_getenv (varname);
> +#elif HAVE_SECURE_GETENV
> +  return secure_getenv (varname);

shouldn't secure_getenv be preferred over the __ version?

(In reply to comment #14)
> shouldn't secure_getenv be preferred over the __ version?

True.

Comment on attachment 67810 [details] [review]
activation-helper: Bypass _dbus_getenv filtering by using getenv

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

(In reply to comment #11)
> Unfortunately, there is one very well-known setuid program that links dbus,
> namely the activation helper. This patch breaks the ability of bus-activated
> services to find their starter bus, because DBUS_STARTER_ADDRESS gets
> cleared.

(Using the syntax $FOO here for environment variables, even though strictly speaking it's not right, to distinguish them from the C constants that often have very similar names...)

I feel as though system-bus-activated services ought to be using dbus_bus_get (DBUS_BUS_SYSTEM, ...) anyway, but you're right that this is really a regression: it used to work, and now it doesn't.

One possible solution would be for the activation helper to set $DBUS_STARTER_ADDRESS to the hard-coded, compile-time-chosen system bus address (DBUS_SYSTEM_BUS_DEFAULT_ADDRESS) instead of inheriting it from the environment. The impact of this would be:

* (good) a subverted dbus-daemon can't escalate its privileges from
  messagebus to root by running the setuid activation helper for a
  root service (e.g. NetworkManager) with a unixexec: address that
  will induce that root service to exec a chosen process

* (bad) a custom dbus-daemon (neither system nor session) configured
  to use the activation helper like a system bus, but listen on an
  unusual address, will pass the wrong $DBUS_STARTER_ADDRESS to
  its children

Mitigation of the bad: it's not at all clear to me that anyone really does this, and if they do, it doesn't work for GDBus applications anyway: passing $DBUS_STARTER_ADDRESS through to GDBus applications is insufficient, because they look at $DBUS_STARTER_BUS_TYPE instead. If it's "session" they use $DBUS_SESSION_BUS_ADDRESS, if it's "system" they use GIO's compile-time-configured equivalent of DBUS_SYSTEM_BUS_DEFAULT_ADDRESS, and otherwise they fail to connect at all.

Comment on attachment 67810 [details] [review]
activation-helper: Bypass _dbus_getenv filtering by using getenv

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

::: bus/activation-helper.c
@@ +149,4 @@
>    const char *debug_env = NULL;
>  
>    /* are we debugging */
> +  debug_env = getenv ("DBUS_VERBOSE");

If you're debugging the activation helper (which needs special compilation options to even *have* this code), you should be debugging a non-setuid copy, so I don't think this line of diff is necessary or desirable.

If you're debugging system services, replacing the system service with a shell script that sets DBUS_VERBOSE and execs the real service seems much safer than passing it through the activation helper.

To be honest I'd be tempted to delete this special case altogether.

(In reply to comment #16)
> One possible solution would be for the activation helper to set
> $DBUS_STARTER_ADDRESS to the hard-coded, compile-time-chosen system bus
> address

Another possibility would be to filter $DBUS_STARTER_ADDRESS in some way: perhaps only let it through if it is of the form "unix:path=XXX" where XXX consists entirely of characters in the set [-A-Za-z0-9_/\*.%] (and in particular, no comma, colon or semicolon).

(In reply to comment #8)
> I actually thought about testing whether getenv ("PATH") == __secure_getenv
> ("PATH") as the poor-man's issetugid() on Linux/glibc, but it's really a
> hack...

Yes; on the other hand, it's only there as a workaround for glibc not giving us a way to check this (issetugid() or equivalent) that isn't specifically flagged as non-API (__libc_enable_secure, which is GLIBC_PRIVATE). It seems rather perverse that with sufficiently new glibc, we have secure_getenv() but not issetugid(), where you have to use (something equivalent to) the latter to know whether it's safe to call execvpe()...

In fact, now I think about it more, the answer is obvious. Before we run dbus-launch, we should check that _dbus_getenv ("PATH") is non-NULL, in addition to using _dbus_check_setuid(). if it isn't, assume we're privileged, and refuse to run it (or at least, refuse to search $PATH). Similarly, before we write "keyrings" to our $HOME, we should check that _dbus_getenv ("HOME") is non-NULL, in addition to _dbus_check_setuid(). Problem solved?

Again, I'm ignoring launchd here, but perhaps it would be safer to make DBUS_ENABLE_LAUNCHD explicitly conditional on HAVE_ISSETUGID so that our assertion that it can only happen on FreeBSD-based Mac OS X is safer. (configure.ac checks for launch.h and launchctl, so in principle you could implement it on Linux and we'd use it.)

> I'm not so sure about absence-of-HOME.  I mean, can't that happen for system
> services e.g.?

Perhaps.

(In reply to comment #16)

> One possible solution would be for the activation helper to set
> $DBUS_STARTER_ADDRESS to the hard-coded, compile-time-chosen system bus
> address (DBUS_SYSTEM_BUS_DEFAULT_ADDRESS) instead of inheriting it from the
> environment. The impact of this would be:

I like this solution - Geoffrey, can you redo your patch?

Updated patch series at http://cgit.freedesktop.org/dbus/dbus/log/?h=wip/setuid-hardening

(In reply to comment #8)
> Any suggestions for *where* to document this?  Toplevel docs for
> dbus/dbus-bus.c?  

That sounds as good a place as any.

(In reply to comment #20)
> (In reply to comment #16)
> 
> > One possible solution would be for the activation helper to set
> > $DBUS_STARTER_ADDRESS to the hard-coded, compile-time-chosen system bus
> > address (DBUS_SYSTEM_BUS_DEFAULT_ADDRESS) instead of inheriting it from the
> > environment. The impact of this would be:
> 
> I like this solution - Geoffrey, can you redo your patch?

Recording this in Bugzilla for the record (discussion was on IRC): I tested your revision of my patch with this change, and confirmed that it works in our environment. I agree with this approach.

(In reply to comment #12)
> Comment on attachment 67803 [details] [review] [review]
> Use-__secure_getenv-if-available.patch
> 
> Review of attachment 67803 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Looks good.

Turns out that this breaks gnome-keyring-daemon, at least in my configuration:

$ getcap /usr/bin/gnome-keyring-daemon 
/usr/bin/gnome-keyring-daemon = cap_ipc_lock+ep

And of course, it can no longer read DBUS_SESSION_BUS_ADDRESS, nor autolaunch.

Until we figure out a plan for this, I'm going to back out the __secure_getenv change, and do a dbus 1.6.8.

(In reply to comment #24)

> Until we figure out a plan for this, I'm going to back out the
> __secure_getenv change, and do a dbus 1.6.8.

TL;DR - defer use of __secure_getenv() indefinitely.


<walters> so...anyone have thoughts on https://bugs.freedesktop.org/show_bug.cgi?id=52202#c24 ?  davidz?  stefw?
<walters> i didn't notice it initially because it was just gnome-keyring-daemon, the rest of the system/session worked
<davidz> walters: I guess my view always been that this class of bugs were application-bugs (e.g. not clearing its environment)... I think it's fine to require setuid or cap-using programs to be careful on their own
<walters> davidz, this is the opposite problem
<walters> oh you mean, if libdbus weren't using __secure_getenv(), it'd be up to the app?
<davidz> yeah
<stefw> walters, gnome-keyring-daemon uses that capability to do keep password memory from being paged.
<walters> stefw, yeah i know why
<davidz> I mean, using __secure_getenv() in the library is there only to make buggy programs (those that don't clear the env themselves) more "secure"....
<walters> the question is, were we to use __secure_getenv(), should we pass DBUS_SESSION_BUS_ADDRESS via some other mechanism?  command line argument?
<davidz> I totally agree it would be nice if it worked but...
<davidz> walters: dbus-launch(1) :-)
* davidz runs
<davidz> but, seriously, making GDBus not spawn dbus-launch(1) (just look itself in the X server, dlopening libX11.so in the process) would fix the problem
<davidz> (but I think avoiding using __secure_getenv() is the way forward.... at least for now)
<walters> well, but that's how libc works (and systemd uses it too)
<walters> dunno
<walters> davidz, were we to not even do euid == uid checking, then we'd have to patch coreutils su and/or pam_systemd
<stefw> walters, is it glibc that checks for caps and treats themany caps and treats them as setuid as
<stefw> groan
<stefw> something (pango) is reordering my words
<walters> stefw, well, no the kernel sets an AT_SECURE auxiliary vectory
<walters> *vector
<walters> glibc looks for it
<stefw> grumble
<stefw> because caps_ipc_lock is hardly setuid()
<walters> that happens on plain old setuid binaries, filesystem caps, *and* SELinux domain transitions
<walters> stefw, see http://git.gnome.org/browse/glib/commit/?id=d6cbb29f598d677d5fc1c974cba6d9f646cff491
<stefw> aha, so that's why davidz was suggesting avoiding __secure_getenv() for now?
<stefw> that would solve the breakage
<walters> that's true
<walters> but on the other hand, if someone changed su/sudo to just have fscaps for CAP_SYS_ADMIN or something, we'd be vulnerable again
<davidz> yeah, the __secure_getenv() stuff was only put in to make buggy D-Bus apps (e.g. not clearing the environment) "secure"
<davidz> my view always been it's better to fix the apps
<davidz> but both views are legitimate of course
<davidz> (I mean, we ended doing both, of course)
<walters> at the moment we're doing euid == uid checking
<walters> and i did some smoke testing with also doing __secure_getenv but only just now noticed it breaks gnome-keyring
<walters> anyways, rough consensus to revert that last bit and do a 1.6.8?
<davidz> works for me, sure
<stefw> the git log of dbus looks really strange for me.
<stefw> i see a "hardening: Use __secure_getev() ..."
<stefw> commit with a patch that has nothing to do with that.
<walters> which commit?
<stefw> http://cgit.freedesktop.org/dbus/dbus/commit/?id=d4379ee8dbbe157db173464530df7c069b6fd86f
<walters> that's because _dbus_getenv -> __secure_getenv 
<stefw> ah
<walters> was follow up from smcv's suggestion from https://bugs.freedesktop.org/show_bug.cgi?id=52202#c7
<stefw> so yes, reverting this seems like it would fix the breakage for now: http://cgit.freedesktop.org/dbus/dbus/commit/?id=d839f027ed2cf320ac8b762ea93ba8af74ef50c6
<walters> right
<walters> i'm also reverting that second one since it's pointless without the earlier one
<stefw> once we have systemd session socket activation i'd like to rework gnome-keyring-daemon's startup considerably
<stefw> and perhaps we can work around this case there.
<walters> user bus would significantly help here
<walters> since the system could provide a secure mechanism to map uid -> address
<stefw> true
<stefw> i think this also has to do with the whole 'gnome-keyring-daemon --start' wanting to pass environment variables to the 'gnome-keyring-daemon --login' started by pam
<stefw> the former is dbus activated
<stefw> sometimes
<stefw> walters, so yes that secure mechanism would help for some of it.
<stefw> but we still want to get LANG and stuff into the latter gnome-keyring-daemon

(In reply to comment #24)
> Turns out that this breaks gnome-keyring-daemon, at least in my
> configuration:
> 
> $ getcap /usr/bin/gnome-keyring-daemon 
> /usr/bin/gnome-keyring-daemon = cap_ipc_lock+ep

In principle, we're right to consider this to be privileged: gnome-keyring-daemon has a privilege that you don't normally (it can prevent arbitrarily large amounts of memory from being swapped out). Having it inherit variables that could cause it to be subverted by your unprivileged processes is pretty much equivalent to granting this capability to your entire session.

(If you're the only logged-in user of your computer then in practice that's no bad thing, of course.)

(In reply to comment #26)
> gnome-keyring-daemon has a privilege that you don't normally (it can prevent
> arbitrarily large amounts of memory from being swapped out). Having it
> inherit variables that could cause it to be subverted by your unprivileged
> processes is pretty much equivalent to granting this capability to your
> entire session.

If the gnome-keyring maintainer's considered opinion is that this is OK, one way to do so would be for it to use g_dbus_connection_new_for_address() (if using GDBus) or dbus_connection_open() + dbus_bus_register() (if using libdbus) instead of the higher-level versions; then it's unambiguously gnome-keyring's responsibility to find its address, and gnome-keyring gets to make any relevant security decisions (like "actually, I trust my caller implicitly").

Created attachment 67977 [details] [review]
activation helper: when compiled for tests, do not reset  system bus address

Otherwise, the tests try to connect to the real system bus, which will
often fail - particularly if you run the tests configured for the default
/usr/local (with no intention of installing the result), in which case
the tests would try to connect to /usr/local/var/run/dbus/system_bus_socket.

---

This was a regression caused by these changes. This particular incarnation of the patch is against my backports of the changes to dbus-1.2 (which I'll test against Debian stable shortly), but it applies cleanly to 1.4 and 1.6.

Comment on attachment 67977 [details] [review]
activation helper: when compiled for tests, do not reset  system bus address

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

Looks reasonable to me.

(In reply to comment #27)
> (In reply to comment #26)
> > gnome-keyring-daemon has a privilege that you don't normally (it can prevent
> > arbitrarily large amounts of memory from being swapped out). Having it
> > inherit variables that could cause it to be subverted by your unprivileged
> > processes is pretty much equivalent to granting this capability to your
> > entire session.
> 
> If the gnome-keyring maintainer's considered opinion is that this is OK, one
> way to do so would be for it to use g_dbus_connection_new_for_address() (if
> using GDBus) or dbus_connection_open() + dbus_bus_register() (if using
> libdbus) instead of the higher-level versions; then it's unambiguously
> gnome-keyring's responsibility to find its address, and gnome-keyring gets
> to make any relevant security decisions (like "actually, I trust my caller
> implicitly").

Mmm...I'm uncertain about making that change to gnome-keyring.  It's true that it would help us migrate to __secure_getenv(), but on the other hand, it would be a bit of code we'd have to remember to change if we attempted to migrate to a user bus.

Note that for gnome-keyring, this issue bleeds over into the GLib one, because it links to gio, and thus is "vulnerable" to e.g. GIO_EXTRA_MODULES.

I've released 1.2.30 with (a backport of) these changes.

setuid/setgid executables using libdbus can be identified with something like:

for i in $(find / -type f -a -perm -4000 ; find / -type f -a -perm -2000) ; do echo $i ; ldd $i|grep dbus ; done

(In reply to comment #32)
> setuid/setgid executables using libdbus can be identified with something
> like:

No, sorry, they can't. Any setuid/setgid executable that loads plugins that might use libdbus also needs to be "on the list".

The most prominent plugin architectures that might conceivably involve libdbus and are likely to be used by a setuid process are PAM and nss.

In particular, PAM has a history of

- being used in setuid executables, e.g. su(8)

- not documenting whether it is considered to be OK to use it in setuid
  situations (e.g. no responses to
  <http://www.redhat.com/archives/pam-list/2013-January/msg00005.html>
  when I asked about this)

- people writing plugins that use D-Bus (pam_systemd, pam_ck_connector,
  pam_dbus, pam_fprintd, etc.; pam_dbus and pam_fprintd use the deprecated
  dbus-glib, which is particularly concerning)

-- 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/71.