[Bug 75602]
Simon McVittie
75602 at bugs.launchpad.net
Mon Mar 11 12:30:29 UTC 2013
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.
--
You received this bug notification because you are a member of Ubuntu
Foundations Bugs, which is subscribed to dbus in Ubuntu.
https://bugs.launchpad.net/bugs/75602
Title:
DBUS Should Support "Session Groups"
(pam_group.so,/etc/security/groups.conf)
Status in D-Bus:
In Progress
Status in “dbus” package in Ubuntu:
Triaged
Bug description:
Binary package hint: dbus
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 from Dapper to a more version of Kubuntu).
On my laptop, I can use (the kludge) "adduser $uname plugdev"... this
won't work in the lab.
To manage notifications about this bug go to:
https://bugs.launchpad.net/dbus/+bug/75602/+subscriptions
More information about the foundations-bugs
mailing list