Re: [Qemu-devel] [PATCH 3/3] qemu-bridge-helper: Take ACL file gid into account

[Daniel P. Berrange]

On Tue, May 30, 2017 at 10:23:35AM +0200, Michal Privoznik wrote:
> There's a problem with the way we currently parse ACL files. The
> problem is, the bridge helper has usually SUID flag set and thus
> runs as root in which case all the ACL files are parsed
> (/etc/qemu/bridge.conf and all other files it includes).
> Therefore, if there's say bob.conf owned by root:bob and I run
> the bridge helper, it doesn't really matter whether I'm in the
> bob group or not. Everything that is allowed to bob group is
> allowed to me too.

Ok so the ACL feature relies on the qemu-bridge-helper not having
privileges to read the files included from the master bridge.conf
file.

The easy way for this to work is if the qemu-bridge-helper binary
is given a filesystem capability eg

   setcap cap_net_admin=ep qemu-bridge-helper

in this mode, the bridge helper always runs with uid=500, gid=500,
so does not have permission to read included files which are group
owned by a different user.

If using SUID mode, the bridge helper would be given a custom
group and SUID privs

   chown root.qemu qemu-bridge-helper
   chmod ug+s qemu-bridge-helper

when it runs, it initially has effective uid=0 and effective gid=qemu

It then uses libcapng to drop privileges and change user ID, so that
it ends up with only cap_net_admin, and effective uid=500 and
effective gid=500 again.

So once again, it will be unable to read the included files which
are group owned by a different user.

IOW, I think everything is working normally.

The only scenario in which you can run SUID, and permissions do
not work would be if you compiled QEMU with libcapng support
disabled.

It would be wise to change the bridge helper to refuse to read
any included files with uid != 0 or gid != 0, if libcapng
support is disabled, so make it clear that permission checking
is inoperative.

Or better yet, just make libcapng mandatory when using the
bridge helper.

> The way this problem is fixed is whenever an ACL file is parsed,
> the group owner of the file is stored among with the rules so
> that it can be compared when evaluating.
> 
> There is one exception though. If an ACL file is owned by root
> group the rules within apply to all groups. This is because
> that's the usual setup currently (the bridge.conf is usually
> owned by root:root) and anybody from root group can plug the
> interface themselves anyway.
> 
> This idea of groups was introduced in bdef79a2994d6f0383 but got
> broken by ditros setting SUID onto the binary.

I don't believe it did, unless distros left out libcapng support
when building it.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|