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 . ::: 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.