[libvirt] [Patch v2 3/3] apparmor: QEMU bridge helper policy updates

Fri Jul 27 20:00:13 UTC 2012

On 07/26/2012 11:54 PM, Corey Bryant wrote:
> On 07/26/2012 10:30 AM, rmarwah at linux.vnet.ibm.com wrote:
>> Quoting Jamie Strandboge <jamie at canonical.com>:
>>> On Mon, 2012-07-09 at 10:22 -0400, rmarwah at linux.vnet.ibm.com wrote:
>>>> Quoting Jamie Strandboge <jamie at canonical.com>:
>>>>
>>>> > On Tue, 2012-07-03 at 12:05 -0400, rmarwah at linux.vnet.ibm.com wrote:
>>>> >> Quoting Jamie Strandboge <jamie at canonical.com>:
>>>> >>
>>>> >> > On Fri, 2012-06-29 at 14:08 -0400, rmarwah at linux.vnet.ibm.com
>>>> wrote:
>>>> >> >> From: Richa Marwaha <rmarwah at linux.vnet.ibm.com>
>>>> >> >>
>>>> >> >> This patch provides AppArmor policy updates for the QEMU bridge
>>>> helper.
>>>> >> >> The QEMU bridge helper is a SUID executable exec'd by QEMU that
>>>> drops
>>>> >> >> capabilities to CAP_NET_ADMIN and adds a tap device to a
>>>> network bridge.
>>>> >> >>
>>>> >> >> Signed-off-by: Richa Marwaha <rmarwah at linux.vnet.ibm.com>
>>>> >> >> Signed-off-by: Corey Bryant<coreyb at linux.vnet.ibm.com>
>>>> >> >> ---
>>>> >> >>  examples/apparmor/libvirt-qemu |   21 ++++++++++++++++++++-
>>>> >> >>  1 files changed, 20 insertions(+), 1 deletions(-)
>>>> >> >>
>>>> >> >> diff --git a/examples/apparmor/libvirt-qemu
>>>> >> b/examples/apparmor/libvirt-qemu
>>>> >> >> index 10cdd36..766a334 100644
>>>> >> >> --- a/examples/apparmor/libvirt-qemu
>>>> >> >> +++ b/examples/apparmor/libvirt-qemu
>>>> >> >> @@ -1,4 +1,4 @@
>>>> >> >> -# Last Modified: Mon Apr  5 15:11:27 2010
>>>> >> >> +# Last Modified: Fri Mar 9 14:43:22 2012
>>>> >> >>
>>>> >> >>    #include <abstractions/base>
>>>> >> >>    #include <abstractions/consoles>
>>>> >> >> @@ -108,3 +108,22 @@
>>>> >> >>    /bin/dash rmix,
>>>> >> >>    /bin/dd rmix,
>>>> >> >>    /bin/cat rmix,
>>>> >> >> +
>>>> >> >> +  /usr/libexec/qemu-bridge-helper Cx,
>>>> >> >> +  # child profile for bridge helper process
>>>> >> >> +  profile /usr/libexec/qemu-bridge-helper {
>>>> >> >> +   #include <abstractions/base>
>>>> >> >> +
>>>> >> >> +   capability setuid,
>>>> >> >> +   capability setgid,
>>>> >> >> +   capability setpcap,
>>>> >> >> +   capability net_admin,
>>>> >> >> +
>>>> >> >> +   network inet stream,
>>>> >> >> +
>>>> >> >> +   /dev/net/tun rw,
>>>> >> >> +   /etc/qemu/** r,
>>>> >> >> +   owner @{PROC}/*/status r,
>>>> >> >> +
>>>> >> >> +   /usr/libexec/qemu-bridge-helper rmix,
>>>> >> >> +  }
>>>> >> >
>>>> >> > Looking at the child profile itself, this seems fine.
>>>> >> >
>>>> >> > However, the Cx transition makes it so that all libvirt-managed
>>>> qemu
>>>> >> > processes are allowed to execute this setuid helper and I wonder
>>>> if that
>>>> >> > is too much? Ie, a guest running under libvirt's NAT wouldn't
>>>> need
>>>> >> > access to this helper at all. I wonder if it would be better to
>>>> adjust
>>>> >> > virt-aa-helper to add this transition and child profile instead
>>>> (the
>>>> >> > child profile could theoretically still be in
>>>> apparmor/libvirt-qemu, but
>>>> >> > this is a bit messy)? Can we determine from the domain XML the
>>>> >> > circumstances when /usr/libexec/qemu-bridge-helper will be used?

Well, with the code in patch 1/3 and 2/3, if the actualinterface is
type='bridge', and libvirt is running unprivileged
(!driver->privileged), and the qemu capabilities has
QEMU_CAPS_NETDEV_BRIDGE, then the qemu-helper will definitely be used.

>>>> If so,
>>>> >> > virt-aa-helper could add the access only then. As a side-benefit,
>>>> >> > handling this in virt-aa-helper allows us at compile-time to
>>>> adjust the
>>>> >> > path to qemu-bridge-helper to use the configured path to the
>>>> binary (ie,
>>>> >> > some distros may not use /usr/libexec).
>>>> >>
>>>> >> Thanks a lot reviewing the apparmor patch. We cannot detemine from
>>>> the
>>>> >> domain XML whether /usr/libexec/qemu-bridge-helper will be used or
>>>> not
>>>> >> because we cannot determine whether we are running as privileged
>>>> user
>>>> >> or not.
>>>> > Hmmm, that's too bad.
>>>> >
>>>> >>  Is there a way we can specify the configured path to the
>>>> >> binary in the current policy we have?
>>>> >
>>>> > Not in a convenient way, no. The policy is intended as example
>>>> policy
>>>> > anyway, and distributions are expected to modify this, so I don't
>>>> think
>>>> > this alone is a blocker.
>>>>
>>>> Right now the only way we can think of is that whenever in domain XML
>>>> we see interface=bridge
>>>> we set the policy for the qemu-bridge-helper even though we don't know
>>>> if the qemu-bridge-helper
>>>> is going to be used or not.
>>>
>>> Ah, well, that would at least be somewhat better and IMHO, worthwhile.
>>
>> Hi Jamie
>> I started testing out the policy generation with the approach that if it
>> checks inteface=bridge then only we generate
>> the qemu-bridge-helper policies. I found 2 issues while trying to do
>> that
>> 1) There are a lot of ways to start bridge helper and the way libvirt is
>> starting it is using -netdev bridge br=br0 and the executable
>> is started by the qemu and not libvirt. So the way I can think of
>> changing the path at compile time is to search for the executable in
>> /usr/. Not being a big fan of this approach for searching the
>> executable.
>
> Can you add a new tag to the domain XML that allows specification of
> the helper executable path?  For example, adding <helper>:
>
> <interface type='bridge'>
>       <source bridge='br0'/>
>       <helper='/usr/libexec/qemu-bridge-helper' />
> </interface>

That's a bit opaque about its meaning, and I'm not sure it has a place
at this location in the XML - who (I mean "what person") is going to
determine the location of the bridge helper, and decide that it's even
needed? Especially since the idea here is to allow unprivileged users to
get full networking functionality, this config will be provided by an
unprivileged user - is there any risk in allowing an unprivileged user
to specify this binary's path? (other than the possibility they'll
misunderstand it and simply get it wrong, that is)

On the other hand, I think it will be useful to have *something* in the
XML that indicates this interface wants to use the qemu-helper method of
attaching to the bridge, for a reason that will come at the end of the
message:

To step back a bit and change the topic slightly (but it leads to the
"reason" I mention above :-):

My reservation about this patch series is that I want to be sure that it
is "future proof" in the eventuality that other methods of achieving the
same goal (proper networking support for guests started by
non-privileged users) become available.

In my opinion, having qemu execute a suid binary isn't the proper way to
solve the problem (however, it's currently the only way available), and
I'm hoping that eventually libvirt can be enhanced to achieve the
desired results in a different manner.

In particular, the problems I see with the qemu helper binary are:

1) A lot of time and trouble has been spent minimizing the capabilities
of qemu in order to reduce the amount of damage that could be done by a
compromised qemu process. The suid binary provides a way to "break out".
True it is a small and limited binary, but "low risk" does not mean "no
risk".

2) the text-file-based ACLs it uses will be cumbersome to manage (and
definitely outside the scope of libvirt). This really should be done by
policykit instead  of home-grown ACL files.

3) the qemu helper binary can only handle creating a tap device and
connecting it to a bridge. It can't handle macvtap interfaces, libvirt's
virtual networks (unless the user just happens to know the name of the
bridge used for that interface), openvswitch bridges, nwfilter rules,
bandwidth limiting, 802.1Qb[gh]... You get the picture - there is a
*lot* of functionality that's off the table if the qemu helper binary is
used (unless it gains a lot of features and becomes hopelessly
intertwined with libvirt, and I don't think anyone wants that).

The way that I think the problem should be solved is this:

1) All of the network-related functionality in the system instance of
libvirt that is used by the qemu, lxc, etc. drivers internal to libvirt
(including the nwfilter subsystem, and everything in bridge_driver.c)
should be in a separate daemon from libvirtd, and made available via RPC
with a public API that uses policykit for authorization/authentication,
with separate selinux policies from libvirtd; maybe call it
"libvirt-networkd".

2) When the system instance of libvirtd is creating a domain, it should
call to libvirt-networkd via this API to do things like create a tap
device, connect it to a bridge, add nwfilter rules, etc.

3) likewise, when a session (unprivileged) instance of libvirt is
creating a domain, it also should call the same APIs, which (after
proper authentication/authorization via policykit) will connect it to
the privileged libvirt-networkd (or whatever its called) to perform the
operation.

The result will be that a guest created by an unprivileged user will
(dependent on policykit granting access) have access to *all* of the
network functionality that is available to a guest started by the system
libvirt instance, including libvirt's virtual networks, macvtap
interfaces, nwfilter rules, bandwidth limiting, etc. And managing who
gets access to what will be done in the same way as any other resource
gated by policykit. Additionally, libvirtd will no longer need many of
the capabilities/policies it currently needs to permit setting up the
networking (just as libvirt-networkd won't need all the
capabilities/policies required by libvirtd to handle other functions of
managing a guest's lifecycle).

So, how does this relate to my desire to see something in the XML
indicating that this interface should use the qemu helper? Well, I'm
thinking ahead to the day when the work I've outlined above is completed
- when it is included in a libvirt release, there will be people who
have guests with network configs that work thanks to the presence of the
qemu-helper support and an entry in the qemu-helper's ACL file. If they
upgrade their libvirt and their config stops working (due to their uid
not being authorized by policykit), they won't be happy. On the other
hand, once this more comprehensive solution is available, it should be
preferred by default. In order for that to be the case, we need to have
some sort of tag in the XML *now* that indicates "use the qemu-helper",
and otherwise pretend that it doesn't exist. That way when those people
upgrade, even though the new method of setting up networking (which may
not work without config changes) will be available, libvirt will
understand it should continue to use the "old" method for this
particular guest.