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

On 07/26/2012 11:54 PM, Corey Bryant wrote: > On 07/26/2012 10:30 AM, rmarwah(a)linux.vnet.ibm.com wrote: >> Quoting Jamie Strandboge <jamie(a)canonical.com&gt;: >>> On Mon, 2012-07-09 at 10:22 -0400, rmarwah(a)linux.vnet.ibm.com wrote: >>>> Quoting Jamie Strandboge <jamie(a)canonical.com&gt;: >>>> >>>>> On Tue, 2012-07-03 at 12:05 -0400, rmarwah(a)linux.vnet.ibm.com wrote: >>>>>> Quoting Jamie Strandboge <jamie(a)canonical.com&gt;: >>>>>> >>>>>>> On Fri, 2012-06-29 at 14:08 -0400, rmarwah(a)linux.vnet.ibm.com >>>> wrote: >>>>>>>> From: Richa Marwaha <rmarwah(a)linux.vnet.ibm.com&gt; >>>>>>>> >>>>>>>> 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(a)linux.vnet.ibm.com&gt; >>>>>>>> Signed-off-by: Corey Bryant&lt;coreyb(a)linux.vnet.ibm.com&gt; >>>>>>>> --- >>>>>>>> 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.