Bug 9328 - DBUS Should Support "Session Groups" (pam_group.so,/etc/security/groups.conf)
Summary: DBUS Should Support "Session Groups" (pam_group.so,/etc/security/groups.conf)
Status: RESOLVED DUPLICATE of bug 103737
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: x86 (IA32) Linux (All)
: high major
Assignee: Havoc Pennington
QA Contact: John (J5) Palmieri
URL:
Whiteboard: probably WONTFIX
Keywords:
Depends on:
Blocks:
 
Reported: 2006-12-13 04:40 UTC by Jonathan Anderson
Modified: 2018-05-09 15:18 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
A patch for _dbus_file_get_contents() to support procfs files that appear to be empty (2.36 KB, patch)
2013-02-28 22:43 UTC, Krzysztof Konopko
Details | Splinter Review
Required plumbing for reading process credentials from procfs (24.24 KB, patch)
2013-03-09 14:33 UTC, Krzysztof Konopko
Details | Splinter Review
Required plumbing for reading process credentials from procfs v. 2 (12.20 KB, patch)
2013-03-17 23:42 UTC, Krzysztof Konopko
Details | Splinter Review

Description Jonathan Anderson 2006-12-13 04:40:03 UTC
In my lab, there are lots of users in an LDAP database. These users are 
assigned to the groups audio,video,plugdev,etc. by pam_group (using 
the /etc/security/groups.conf file). The problem is, DBUS doesn't recognize 
users as being in groups unless they're in /etc/group, which isn't practical 
for our setup. So, there are permission problems with USB sticks, kpowersave, 
etc. (well, there *will* be, once I upgrade to a more recent KDE version).

On my laptop, I can use (the kludge) "adduser $uname plugdev"... this won't 
work in the lab.
Comment 1 Havoc Pennington 2006-12-13 05:00:23 UTC
dbus just uses the normal C library functions to get the user's groups; it 
isn't manually groping around in /etc/group or anything.

Maybe you need to configure libc to use nscd or something like that?

I don't know how to fix this on the dbus level, certainly dbus should not be 
in the business of understanding NIS, LDAP, PAM, /etc/group, and so forth by 
hand...
Comment 2 Jonathan Anderson 2006-12-13 06:34:25 UTC
Okay, so I did some digging in the DBUS source... I think that there are two 
problems, one libc-related, the other DBUS-related.

The first is that getgrouplist() does not return "dynamic" groups assigned by 
pam_group. This is not a DBUS issue, so I guess it'll have to be a libc/PAM 
wishlist item.

The second is that (if I'm reading this correctly) DBusUserDatabase *system_db 
is built when the daemon starts up, so it can only use static group 
assignments. If getgrouplist() were modified (or an alternative found), could 
DBUS be modified to allow group assignments to be "freshened"?
Comment 3 Havoc Pennington 2006-12-13 08:35:10 UTC
There is a patch in CVS now that allows you to build in a mode where groups 
aren't cached. The problem is that if you're using the read-/etc-group libc 
implementation instead of nscd this will be absurdly performance-intensive.
That is, every time someone sends a message over the bus we'd load and 
parse /etc/group and /etc/passwd.

What we'd like someone to do is write a small test program to benchmark group-
based bus security policy checks with and without caching the user/group 
information, and then run that benchmark 
a) with current caching
b) with regular "read the file" libc implementation
c) with nscd implementation

Then we can discuss the right course of action.

More discussion can be found in the mailing list archives.
Comment 4 Jerome Charaoui 2007-10-27 15:21:33 UTC
I haven't found much about this bug in the mailing list archive... Was a workaround found? Otherwise, has there been any progress on this front?

Using Debian, I find it somewhat tedious to have to add every user to groups such as audio, plugdev and netdev in /etc/group on a multi-user desktop machine, to enable basic functionality like sound, automounting and network-manager access.
Comment 5 Havoc Pennington 2007-10-28 01:03:52 UTC
I don't know of any progress. The situation last I knew remained that with /etc/group it is absurdly expensive not to cache the info from libc; with nscd, I'm not sure any profiling has been done. I don't know how the common distributions configure by default in recent versions.

By hacking the dbus code you can trivially disable the caching of group information, but the question is just whether that will slow performance to a crawl. Remember the stuff dbus is doing happens for *every message* - it has to check what groups the user sending the message is in. Opening a file or making a blocking network request for every message will be too expensive.

This may or may not be hard to fix, or may or may not even be a real problem with "normal" configurations shipped by distributions these days. All I really know for sure is that when originally implementing this feature (on some years-old Fedora or Red Hat flavor), the caching was necessary to avoid parsing /etc/groups over and over.

So, what we need is research first of all.

If it is still too slow to turn off the cache, perhaps a fix would be to expire the cache after some period of time. However I would not spend time on that without first just documenting the situation (what libc is doing, and how fast it is, in typical configurations).
Comment 6 Dariem Pérez Herrera 2008-01-20 12:54:36 UTC
I've configured an Ubuntu box to authenticate using Microsoft Active Directory through pam_winbind.so. Domain users can use pluggable devices through dbus/hal because I've used pam_group.so to assign such users to plugdev group. It worked just fine. Now I'm using Gentoo and I can't do the same, it's not working. Is it possible that some Ubuntu developer have solved the problem? My partial solution in Gentoo was to configure /etc/dbus-1/system.d/hal.conf to assign priviledges to group "domain users" and to modify /etc/init.d/dbus to start after samba/winbind. But there is a problem: any short-timed failure on network connection prevents dbus for knowing who are "domain users", so this users can't automount pluggable devices after any of these failures, they have to wait for my presence so I can restart dbus daemon, and consequently, all GNOME environment. I think this issue should be solved as soon as possible, because is fatal for remote authentication mechanism (LDAP, Kerberos, Active Directory) when network users can't use something as common as a pendrive. 
Comment 7 C G 2009-11-27 01:52:07 UTC
From comment #6 i got the idea that /etc/group is cached and read only once during start up or simmilar.

Would it be possible to refresh the cache when a request is made for a new user, that previously wasn't queried before?

Or maybe only rely on chache for new users after some amount of seconds has passed without changes (due to session scripts).
Comment 8 Colin Walters 2009-11-30 07:44:11 UTC
Note applications should migrate to PolicyKit for privileged operations, and avoid using the DBus security policy.

In the specific case of HAL, work is ongoing already to replace it.

That all said, my guess is that the easiest fix here is to enable nscd and disable dbus' built in caching.  (Maybe the latter could be a flag in the init script or we somehow detect nscd running)
Comment 9 Simon McVittie 2011-02-23 07:09:51 UTC
The problem here is that the Unix semantics of groups are rather non-obvious. Each Unix process has a primary group ID and an array of of supplementary group IDs; these are *normally* the group ID and supplementary groups of the process's owning user, but things like pam_group cause that not to be true.

dbus-daemon accesses the system group database via libc. On a GNU system, that means nsswitch (a module of glibc); you can access that database from the command line  by running "getent groups" (which returns something that looks remarkably similar to /etc/group, but also contains information from LDAP or whatever).

When login(1), su(1) or whatever switches uid from root to a user, it reads from the system's group database, and assigns those groups to the process. This has some interesting side-effects; for instance, if you remove a user from, say, the audio group, but they have a background process like a screen(1) session, that process still has audio rights. Similarly, if you add a user to the audio group, any of their processes that are already running won't automatically get audio rights (until they run sg or newgrp or something).

pam_group doesn't add users to groups - it adds extra groups to processes. dbus-daemon starts from the uid and reads directly from the system group database to determine the groups, so it can't see those extra groups.

Using or not using the libdbus "userdb cache" won't fix this, because it's still reading from nsswitch and not looking at the requesting process's credentials directly.

Using or not using ncsd won't fix this either, because ncsd returns the same things as nsswitch (albeit perhaps an out-of-date version).

"Fixing" this would be not at all trivial. The D-Bus wire protocol normally uses a single Unix credential-passing message (on first connection to the bus) to get the user ID, but credential-passing can normally only carry one user ID and one group ID - so dbus-daemon never finds out the list of supplementary groups, only the primary group.

I'm inclined to say this is WONTFIX, because:

* using PolicyKit solves this for recent versions of HAL, upower, udisks,
  etc. as used in current GNOME (and presumably also KDE) distributions,
  removing the need for plugdev, powerdev, etc. group membership

* not directly related to D-Bus, but using ConsoleKit and udev also solves
  this for device-node ownership: on my Debian sid laptop, logging in to
  a local GNOME session automatically gives me rw access to appropriate
  device nodes via a POSIX ACL, for instance:

  # file: dev/snd/pcmC0D0p
  # owner: root
  # group: audio
  user::rw-
  user:smcv:rw-
  group::rw-
  mask::rw-
  other::---

* using pam_group for temporary group membership is generally insecure,
  in the sense that there's trivial privilege escalation from temporary
  to permanent group membership if users can write to any location that
  supports setgid executables:

  * log in at the console and be placed in the audio group

  * cd to anywhere that supports setgid executables (home directories
    are usually sufficient)

  * cp /bin/bash audiobash && chgrp audio audiobash && chmod g+s audiobash

  * log out

  * ssh in while another user is logged-in at the console

  * run audiobash, giving you a shell process in the audio group

  * use your new write access to /dev/snd to switch on the microphone and
    listen to the what the console user is doing
Comment 10 C G 2011-02-28 23:14:08 UTC
Thank you for the summary.

Did find this thread about the issue:
http://web.archiveorange.com/archive/v/2mxmYMz9oyt4896NAcKZ
(http://lists.freedesktop.org/archives/dbus/2008-August/010191.html)

Concluding, it looks like there is some code/patch available to allow to check for the correct full list of groups (incl. dynamicly added) of a process at connect time.
Comment 11 Simon McVittie 2012-02-08 06:47:40 UTC
From that mail:
> The solution is a bit hairy, because it does require a changed kernel
> (at least I haven't found any other way to test the group of another
> process efficiently).

There's also an interesting attack based on a race condition, if dbus-daemon performs access control by asking the kernel for a process's credentials by pid:

* attacker has (say) uid 1001, gid 11111 and pid 12345

* attacker sends a D-Bus message which is only allowed for group 22222
  (or for uid 0, for that matter)

* [race] attacker immediately uses exec() to replace itself with any
  setuid-0 or setgid-22222 binary

* [race] dbus-daemon asks the kernel "what is the gid of pid 12345?"
  (or for the uid 0 case, "what is the uid of pid 12345?"

* if the dbus-daemon wins the race, the kernel replies "11111"
  (or 1001) and permission is denied

* if the attacker wins the race, the kernel replies "22222" (or 0)
  and permission is granted!

... so, don't do that.

For local Unix sockets on Linux, the credentials exchange step at the beginning of the connection means that dbus-daemon can validate the connection's uid, pid and one of its groups - but there's no way to get the other groups, unless the connecting process sends one message per group. This would require modifications to dbus-daemon and all client libraries wishing to use this mechanism (libdbus, QtDBus, GDBus, etc.), and wouldn't work for non-local connections, or on platforms where this credentials-passing doesn't work.

A new syscall of the form "what are the uid, gid and all supplementary groups at the other end of this socket?" or "does the other end of this socket have these credentials?" would also solve this, albeit in a completely non-portable way.
Comment 12 Krzysztof Konopko 2013-02-28 22:28:42 UTC
Here's a mailing list thread that alluded to this bug:
http://lists.freedesktop.org/archives/dbus/2013-February/015505.html

The idea is to at least try harder on Linux where relevant user/group information can be extracted from procf status file.

I'll be sending some patches here for review to see if it's plausible to take this approach.
Comment 13 Krzysztof Konopko 2013-02-28 22:43:07 UTC
Created attachment 75714 [details] [review]
A patch for _dbus_file_get_contents() to support procfs files that appear to be empty

This is the first patch in hopefully a series of patches that will try to remedy the problem described in this bug at least on Linux.

First thing to do is to support files in procfs that appear to be empty.  Unfortunately _dbus_file_get_contents() ignores such files upfront.

The patch has been inspired by a patch used in a production system:
https://github.com/kkonopko/dbus/commit/7af563d558808fc91d181b5bf9fe24543a44df4c#L8R148

This one uses _dbus_read() instead of plain read.  It's also tempting to fold the logic from the last `else' statement into the first one.  And even further the logic that closes the file (and reverts the output DBusString size) and returns the status could also be folded.  This will require writing some tests for this function first.

The patch is to get a brief opinion and views.
Comment 14 Krzysztof Konopko 2013-03-09 14:33:00 UTC
Created attachment 76224 [details] [review]
Required plumbing for reading process credentials from procfs
Comment 15 Simon McVittie 2013-03-11 12:30:29 UTC
Comment on attachment 76224 [details] [review]
Required plumbing for reading process credentials from procfs

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

This is a much more intrusive change than you actually need, and appears to aim to change functions' semantics in ways that make their names no longer make sense.

A general comment for anyone who wants to alter D-Bus access-control: be aware of the attack involving exec() described in <https://bugs.freedesktop.org/show_bug.cgi?id=47581#c3>.

::: dbus/dbus-auth.c
@@ +549,5 @@
>      }
>        
> +  if (!_dbus_credentials_add_from_user (auth->desired_identity,
> +                                        data,
> +                                        _dbus_credentials_get_unix_pid (auth->credentials)))

This is in DBUS_COOKIE_SHA1 authentication, which is used if we don't have enough credentials-passed information for EXTERNAL authentication. Why would it ever know the pid?

(Also, if it can legitimately know the pid, the same comments as below apply.)

@@ +1080,5 @@
>    else
>      {
>        if (!_dbus_credentials_add_from_user (auth->desired_identity,
> +                                            &auth->identity,
> +                                            _dbus_credentials_get_unix_pid (auth->credentials)))

I think this may be the only place where you actually need the pid...

Extending _dbus_credentials_add_from_user() is inappropriate, given the name of the function. Instead, you should add _dbus_credentials_add_from_other_process (DBusCredentials *credentials, dbus_pid_t pid) or something. Make sure it can return "I don't know", as it usually will on non-Linux. If it does, this caller should fall back to some other strategy, for instance calling _dbus_credentials_add_from_user() like it does now.

Here's an interesting design question: a connecting process sends us "I am { uid 1000, pid 1234 }" via credentials-passing, and uses the EXTERNAL SASL mechanism to say "I am uid 1000". We look in /proc/1234 to find out its groups. It turns out to have uid 2000 (which must mean it made use of CAP_SETUID, or exec()'d a setuid process). What should happen? What are its credentials now?

I don't think "its uid is assumed to be 2000" is an acceptable answer: during the SASL handshake, it explicitly told us that it wanted to authenticate as uid 1000!

It's possible that the "we have /proc" code path might need to use the username/uid as well (because it's the SASL identity). Perhaps it needs to call _dbus_credentials_add_from_user() *and* _dbus_credentials_add_from_other_process().

Another interesting design question: suppose a connecting process sends us "I am { uid 1000, pid 1234 }" via credentials-passing, and uses the EXTERNAL SASL mechanism to say "I am uid 1000". /etc/passwd and /etc/group say that uid 1000 is a member of groups 100 and 101. When we inspect /proc/1234 we discover that it has uid 1000 and groups 100 and 200. What are the connections's credentials? Current libdbus would say { uid=1000, pid=1234, group 100, group 101 }. They definitely include uid=1000, pid=1234. This bug report wants them to include { group 100, group 200 } but do they also include group=101?

::: dbus/dbus-connection.h
@@ +142,4 @@
>   */ 
>  typedef dbus_bool_t (* DBusAllowUnixUserFunction)  (DBusConnection *connection,
>                                                      unsigned long   uid,
> +                                                    unsigned long   pid,

This is a public API break (an API break in a header that gets installed), which is not acceptable.

In any case, why would a DBusServer implementor ever want the decision whether to accept a new connection to depend on the pid?

::: dbus/dbus-sysdeps-unix.c
@@ +2273,5 @@
> +                          DBusError    *error)
> +{
> +  dbus_bool_t ret = FALSE;
> +
> +  return ret;

As noted in the long commit message, this is still a stub. Stub implementations should say so (a comment or a #warning or something).

::: dbus/dbus-sysdeps-util-unix.c
@@ +979,4 @@
>                              dbus_gid_t          **group_ids,
>                              int                  *n_group_ids)
>  {
> +  return _dbus_groups_from_uid (uid, pid, group_ids, n_group_ids);

Why would a function called _dbus_unix_groups_from_uid() or _dbus_groups_from_uid() ever need to know a process ID? It should always do what its name says: given a uid, look it up in /etc/passwd and /etc/group (or equivalent), and return its groups.

In the "session groups" case, you're going to need to have a separate function called _dbus_unix_groups_from_pid() or something; _dbus_unix_groups_from_uid() should probably only be called if _dbus_unix_groups_from_pid() returns "I don't know".

@@ +997,3 @@
>                                 DBusError         *error)
>  {
> +  return _dbus_is_console_user (uid, pid, error);

Why would a function called either _dbus_unix_user_is_at_console() or _dbus_is_console_user() ever need to know a process ID? Either uid is logged-in on some local console, or they are not. The pid has nothing to do with it.

::: dbus/dbus-userdb.c
@@ +131,2 @@
>                              const DBusString *username,
>                              DBusError        *error)

DBusUserDatabase represents /etc/passwd and /etc/group (or the OS's equivalent, e.g. the nsswitch mechanism in glibc). It shouldn't have anything to do with process IDs.

::: dbus/dbus-userdb.h
@@ +119,4 @@
>                                                   DBusString        *homedir);
>  
>  dbus_bool_t _dbus_homedir_from_uid              (dbus_uid_t         uid,
> +                                                 dbus_pid_t         pid,

Why would a function called _dbus_homedir_from_uid() ever need to know a process ID? It should always do what its name says: given a uid, look it up in /etc/passwd or equivalent, and return its home directory.
Comment 16 Krzysztof Konopko 2013-03-12 09:50:21 UTC
Comment on attachment 76224 [details] [review]
Required plumbing for reading process credentials from procfs

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

Thanks for your really valuable comments and your time.  I admit this PID thing is sprinkled everywhere.  I didn't want to spend too much effort without knowing your views first and this review proves that I would have gone astray.  I'll revisit this patch and address all your concerns focusing only on EXTERNAL authentication and supplementary groups.  My other goal is also try to not even touch /etc/{passwd,group} at all if not absolutely necessary.

Please see also my specific comment wrt design/security.

::: dbus/dbus-auth.c
@@ +1080,5 @@
>    else
>      {
>        if (!_dbus_credentials_add_from_user (auth->desired_identity,
> +                                            &auth->identity,
> +                                            _dbus_credentials_get_unix_pid (auth->credentials)))

I'll address only specific questions here and defer the rest to the generic comment.

If an account has access to setuid/setgid executables and is available to potentially rogue users, then the system is flawed, not D-Bus.  IMHO the vulnerability here are setuid/setgid programs that shouldn't exist at all or at least shouldn't be available to random users.  It's a bit like saying "Everyone on the system has passwordless sudo access because a sysadmin/architect had a bad day.  Oops!  Let's defend!". 

I don't understand your reasoning in one of your comments [1]:
* cp /bin/bash audiobash && chgrp audio audiobash && chmod g+s audiobash

The user must be a member of `audio' group in order to chgrp a file to that group.  Why then ever bother with these tricks if the user is a *genuine* member of the group?

You also mentioned in your comment on some other bug [2]:
/* any setuid program will do */
exec ("/bin/ping", "example.com")

Again, no setuid/setgid should be available.  In this instance `ping' program should have cap_net_admin and cap_net_raw capabilities set.  No UID change.

[1] https://bugs.freedesktop.org/show_bug.cgi?id=9328#c9
[2] https://bugs.freedesktop.org/show_bug.cgi?id=47581#c3

----
The reality is different though and no one wants to make libdbus the culprit.  So I agree with you:  UID should be received from the socket rather than from procfs.

WRT the other question where you consider different groups in /etc/group and in procfs, my opinion is that those in /etc/group should be ignored.  My view is that someone knew better and since they were privileged enough to convince kernel to set those groups on the process, libdbus in this instance should respect that.

Now again you could argue that there's a race condition here and someone was able to gain group membership the same way as for UID.  I think it's up to the system administrator/architect to provide or deny such opportunity.  For example the system I work on has all D-Bus services/clients running in their individual sandboxes where one of the features is a separate mount namespace with all filesystems mounted either ro,nosuid or noexec.

So the conclusion is that this functionality could be optional controlled in a twofold manner:
* compile/build time
  This is rather obvious since procfs is not available on all systems.  I guess __linux__
  macro would do.

* D-Bus configuration
  If someone is able to configure D-Bus to use this feature (presumably this would be a
  system administrator) then they are confident enough about their system and should be
  allowed to use this feature.

What do you think?
Comment 17 Simon McVittie 2013-03-12 12:31:32 UTC
(In reply to comment #16)
> I'll revisit this patch and address all your concerns focusing
> only on EXTERNAL authentication and supplementary groups.  My other goal is
> also try to not even touch /etc/{passwd,group} at all if not absolutely
> necessary.

Great. If you can get this working well and securely, I'm happy to review/merge.

Regarding setuid:

> If an account has access to setuid/setgid executables and is available to
> potentially rogue users, then the system is flawed, not D-Bus.

They don't have to be setuid executables that do anything inherently unsafe on their own. su, sudo or pkexec is enough for the attack I described: as long as its uid is 0 at the time that the dbus-daemon looks at it, it doesn't matter that all it was doing is sitting at an "enter password:" prompt.

Even if *your* system does not have any setuid executables, general-purpose Unix does - su, sudo, pkexec are easy examples - and D-Bus is meant to be functional and secure on general-purpose Unix, and in particular, general-purpose Linux.

Now, this attack is mostly about what happens if you check credentials per-message instead of per-connection, which you're not doing, so the most straightforward version of the attack is avoided. I'm working on the assumption that for bad things to happen, the client will have to send a message. If it exec()s a privileged executable, it "has lost control" and can no longer send "bad messages".

However, I'm concerned about the possibility of a client queueing up its side of the entire authentication handshake, the Hello message, and a "bad message" into the socket's buffer, then exec()ing; is it possible that the server could perform the /proc check, see the exec()'d executable's credentials, and then act on the rest of the pre-queued data from the *original* executable?

One way to avoid this would be to avoid the /proc stuff, and instead extend the authentication handshake with a new verb similar to NEGOTIATE_UNIX_FD:

    C: AUTH EXTERNAL 31303030
    S: OK
    C: I_AM_IN_GROUP 100
    S: OK
    C: I_AM_IN_GROUP 101
    S: OK
    C: BEGIN

where the first message of I_AM_IN_GROUP is accompanied by SCM_CREDENTIALS out-of-band data containing the corresponding gid. That's likely to be somewhat more difficult than rummaging in /proc, though.

> * cp /bin/bash audiobash && chgrp audio audiobash && chmod g+s audiobash
> 
> The user must be a member of `audio' group in order to chgrp a file to that
> group.  Why then ever bother with these tricks if the user is a *genuine*
> member of the group?

Please read Comment #9 if you're unfamiliar with Unix group semantics - they are not what you might think they are.

If a user is not (according to /etc/group) a member of audio, but their shell has the audio supplementary group due to pam_group, then they can escalate from "temporarily in audio group" to "permanently able to get into audio group" by copying a shell (or anything similar) and chmod'ing it g+s. This makes session groups not very useful as a security measure on general-purpose Unix (they can be secure on a system where users cannot write to any filesystem mounted without nosuid and/or noexec, but D-Bus is not restricted to those systems).

> Again, no setuid/setgid should be available.  In this instance `ping'
> program should have cap_net_admin and cap_net_raw capabilities set.  No UID
> change.

I admit that ping is a bad example; substitute anything that, in order to perform its desired function, has to be setuid-root and world-executable (or setgid to a desired group, for group-based permissions). su is a particularly prominent example.

> WRT the other question where you consider different groups in /etc/group and
> in procfs, my opinion is that those in /etc/group should be ignored.  My
> view is that someone knew better and since they were privileged enough to
> convince kernel to set those groups on the process, libdbus in this instance
> should respect that.

Fair enough. This is the option that's more likely to break existing applications (which could be relying on the "I get my uid's groups from /etc/group" semantics), but I don't think it'll break anything in practice.

> Now again you could argue that there's a race condition here and someone was
> able to gain group membership the same way as for UID.

Perhaps. If there is one, please address it, if not, convince me of that fact?

> I think it's up to
> the system administrator/architect to provide or deny such opportunity.  For
> example the system I work on has all D-Bus services/clients running in their
> individual sandboxes where one of the features is a separate mount namespace
> with all filesystems mounted either ro,nosuid or noexec.

That's great for your system, but in a general-purpose Unix distribution this is rarely an option, and I'm not willing to make D-Bus insecure when used in a general-purpose distribution.

> * D-Bus configuration
>   If someone is able to configure D-Bus to use this feature (presumably this
> would be a
>   system administrator) then they are confident enough about their system
> and should be
>   allowed to use this feature.

I'm reluctant to add configuration options that are only safe to use on heavily-locked-down systems, and when enabled on a general-purpose Unix system, subtly undermine system security. Security is quite hard enough already without my help :-)
Comment 18 Simon McVittie 2013-03-12 12:41:57 UTC
(In reply to comment #17)
> If you can get this working well and securely, I'm happy to
> review/merge.

On the other hand, I still think a more appropriate solution to the problem you outlined on the mailing list and in your git repository (dynamically-allocated users/groups without editing /etc) would be to install a nss module that understands those users/groups; or if your libc doesn't have pluggable nss, modify your libc to know about them directly, like Android's Bionic libc.

If this feature request can't be satisfied in a way that is clearly secure, I'm afraid I would prefer not to satisfy it at all.
Comment 19 Krzysztof Konopko 2013-03-17 23:42:26 UTC
Created attachment 76666 [details] [review]
Required plumbing for reading process credentials from procfs v. 2

OK, here's another attempt with the PID plumbing :)

This time I tried to focus on EXTERNAL authentication and took your advice from the previous review.  My main goal is to avoid touching /etc/{passwd,group} if possible.

Here are the highlights:
- if the data sent along with AUTH message is a numeric UID, then try to use it instead of looking in /etc/passwd.
See handle_server_data_external_mech().  The desired_identity is checked against the UID from the socket so no risk here.

- the next step is determining whether the client can connect
bus_policy_allow_unix_user() takes additionally PID as an argument and tries to get groups from /proc first (_dbus_unix_groups_from_pid() not implemented yet) and falls back to the lookup based on UID.

- a new API function introduced to get the PID form the connection and pass it to client authentication: dbus_connection_get_unix_process_id_unauth()
As opposite to dbus_connection_get_unix_process_id(), it doesn't try to authenticate the user internally which at this stage would end up in the endless loop.

Unfortunately got stuck with bus_policy_create_client_policy()
This function calls _dbus_unix_user_is_at_console() which unfortunately needs a username so ends up looking in /etc/passwd (for UIDs without the entry in /etc/passwd _dbus_user_database_lookup() fails with assertion).  Need to think how to avoid touching /etc/passwd here.
The goal here is to let D-Bus find out about the process from it's state (socket credentials, procfs if possible) rather than pesky /etc/passwd.  And a natural way of doing it is to try the authentication with UIDs not present in /etc/passwd.
I guess the priority in this bug is to focus on groups.  So if I can't come up with something reasonable for the problem above, I'll carry on with groups only.
Comment 20 Simon McVittie 2014-09-10 15:40:35 UTC
Comment on attachment 76666 [details] [review]
Required plumbing for reading process credentials from procfs v. 2

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

This is OK up to a point, but not useful without going the rest of the way to the desired feature, which I don't think is implementable in a secure way.

::: dbus/dbus-connection.c
@@ +5203,5 @@
> + * Returns #TRUE if the pid is filled in.
> + *
> + * @param connection the connection
> + * @param pid return location for the process ID
> + * @returns #TRUE if uid is filled in with a valid process ID

"if pid is"

@@ +5206,5 @@
> + * @param pid return location for the process ID
> + * @returns #TRUE if uid is filled in with a valid process ID
> + */
> +dbus_bool_t
> +dbus_connection_get_unix_process_id_unauth (DBusConnection *connection,

What makes this differ from dbus_connection_get_unix_process_id()?

(Yes I know I could find out from the implementation, but if I have to do that, then the doc-comment is insufficient.)

::: dbus/dbus-sysdeps-util-unix.c
@@ +986,5 @@
> +                            dbus_gid_t       **group_ids,
> +                            int               *n_group_ids)
> +{
> +  /* TODO: Implement this function */
> +  return FALSE;

I still don't think this can be implemented securely in current Linux, and I am not willing to merge an insecure implementation. Sorry.
Comment 21 Simon McVittie 2014-09-10 15:45:19 UTC
(In reply to comment #8)
> applications should migrate to PolicyKit for privileged operations

(In reply to comment #18)
> I still think a more appropriate solution to the problem
> you outlined on the mailing list and in your git repository
> (dynamically-allocated users/groups without editing /etc) would be to
> install a nss module that understands those users/groups; or if your libc
> doesn't have pluggable nss, modify your libc to know about them directly,
> like Android's Bionic libc

So, sorry, this is WONTFIX, unless someone turns up with an obviously-secure implementation, and in particular, explains how it is immune to the attack in Comment #11. In practice, I think that would require kernel enhancements (the systemd developers want to add more SCM_ things, which would certainly help).
Comment 22 Krzysztof Konopko 2014-09-10 17:45:30 UTC
(In reply to comment #21)
> (In reply to comment #8)
> > applications should migrate to PolicyKit for privileged operations
> 
> (In reply to comment #18)
> > I still think a more appropriate solution to the problem
> > you outlined on the mailing list and in your git repository
> > (dynamically-allocated users/groups without editing /etc) would be to
> > install a nss module that understands those users/groups; or if your libc
> > doesn't have pluggable nss, modify your libc to know about them directly,
> > like Android's Bionic libc
> 
> So, sorry, this is WONTFIX, unless someone turns up with an obviously-secure
> implementation, and in particular, explains how it is immune to the attack
> in Comment #11. In practice, I think that would require kernel enhancements
> (the systemd developers want to add more SCM_ things, which would certainly
> help).

Yes, I understand what I proposed might not be acceptable.  And I agree it's very difficult or impossible to come up with a proper solution given the current Linux state of affairs.

And in my case the situation is even worse as I'm stuck with quite oldish kernel and have no control over libc (uClibc).  What I proposed here is the best I could come up with given the circumstances and wondered how much of it could be useful for D-Bus.  Quite possibly nothing can be done any better in my case and possibly the problem should be solved elsewhere given enough freedom to choose (and control over) the software.  Tweaking libc (uClibc) seems the most plausible option.

At least quite enlightening discussion was provoked.  Thanks for reviewing and sharing your thoughts.  I'm not in the best the place with all of this but at least now I know why I'm there.

Cheers,
Kris
Comment 23 Simon McVittie 2018-05-09 15:18:22 UTC
(In reply to Simon McVittie from comment #21)
> In practice, I think that would require kernel enhancements
> (the systemd developers want to add more SCM_ things, which would certainly
> help).

These kernel enhancements happened in Linux 4.13, and dbus 1.13.4 makes use of them (Bug #103737).

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


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.