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.
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.)
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?
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".
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.
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.
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 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.freedeskt op.org/ show_bug. cgi?id= 47581#c3>.
::: dbus/dbus-auth.c credentials_ add_from_ user (auth-> desired_ identity, ls_get_ unix_pid (auth-> credentials) ))
@@ +549,5 @@
> }
>
> + if (!_dbus_
> + data,
> + _dbus_credentia
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 @@ credentials_ add_from_ user (auth-> desired_ identity, ls_get_ unix_pid (auth-> credentials) ))
> else
> {
> if (!_dbus_
> + &auth->identity,
> + _dbus_credentia
I think this may be the only place where you actually need the pid...
Extending _dbus_credentia ls_add_ from_user( ) is inappropriate, given the name of the function. Instead, you should add _dbus_credentia ls_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_credentia ls_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_credentia ls_add_ from_user( ) *and* _dbus_credentia ls_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 erFunction) (DBusConnection *connection,
@@ +142,4 @@
> */
> typedef dbus_bool_t (* DBusAllowUnixUs
> 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 from_uid (uid, pid, group_ids, n_group_ids);
@@ +979,4 @@
> dbus_gid_t **group_ids,
> int *n_group_ids)
> {
> + return _dbus_groups_
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 @@ console_ user (uid, pid, error);
> DBusError *error)
> {
> + return _dbus_is_
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 from_uid (dbus_uid_t uid,
@@ +119,4 @@
> DBusString *homedir);
>
> dbus_bool_t _dbus_homedir_
> + 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.