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

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

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?

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/75602

Title:
  DBUS Should Support "Session Groups"
  (pam_group.so,/etc/security/groups.conf)

To manage notifications about this bug go to:
https://bugs.launchpad.net/dbus/+bug/75602/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

Reply via email to