Comment 17 for bug 75602

Comment on attachment 76224
Required plumbing for reading process credentials from procfs

Review of attachment 76224:
-----------------------------------------------------------------

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.