Re: [libvirt] [PATCH] security: apparmor: make vhost-net access a static rule

On Mon, Mar 4, 2019 at 11:42 AM Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; wrote: > > On Fri, Mar 1, 2019 at 5:56 PM Jamie Strandboge <jamie(a)canonical.com&gt; wrote: > > > > On Mon, 18 Feb 2019, Christian Ehrhardt wrote: > > > > > So far we were detecting at guest start if any devices needed vhost net > > > and only if that was true added a rule for /dev/vhost-net. > > > > > > It turns out that it is an absolutely valid case to start a guest > > > without any vhost-net networking but later on wanting to hotplug such a > > > device which then would be denied by apparmor. > > > > > > Unfortunately there also is no security labeling callback involved other > > > than the one to /dev/net/tun. But on the other hand vhost-net is no more > > > new and considered rather safe. Therefore drop the old detection and > > > just add it as a static rule. > > > > > > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1815910 > > > > > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; > > > --- > > > src/security/apparmor/libvirt-qemu | 1 + > > > src/security/virt-aa-helper.c | 17 +---------------- > > > 2 files changed, 2 insertions(+), 16 deletions(-) > > > > > > diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu > > > index eaa5167525..a71f34c175 100644 > > > --- a/src/security/apparmor/libvirt-qemu > > > +++ b/src/security/apparmor/libvirt-qemu > > > @@ -21,6 +21,7 @@ > > > signal (receive) peer=/usr/sbin/libvirtd, > > > > > > /dev/net/tun rw, > > > + /dev/vhost-net rw, > > > /dev/kvm rw, > > > /dev/ptmx rw, > > > /dev/kqemu rw, > > > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c > > > index 8e22e9978a..ebc4feac77 100644 > > > --- a/src/security/virt-aa-helper.c > > > +++ b/src/security/virt-aa-helper.c > > > @@ -937,7 +937,7 @@ get_files(vahControl * ctl) > > > size_t i; > > > char *uuid; > > > char uuidstr[VIR_UUID_STRING_BUFLEN]; > > > - bool needsVfio = false, needsvhost = false; > > > + bool needsVfio = false; > > > > > > /* verify uuid is same as what we were given on the command line */ > > > virUUIDFormat(ctl->def->uuid, uuidstr); > > > @@ -1248,21 +1248,6 @@ get_files(vahControl * ctl) > > > } > > > } > > > > > > - if (ctl->def->virtType == VIR_DOMAIN_VIRT_KVM) { > > > - for (i = 0; i < ctl->def->nnets; i++) { > > > - virDomainNetDefPtr net = ctl->def->nets[i]; > > > - if (net && net->model) { > > > - if (net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_QEMU) > > > - continue; > > > - if (!virDomainNetIsVirtioModel(net)) > > > - continue; > > > - } > > > - needsvhost = true; > > > - } > > > - } > > > - if (needsvhost) > > > - virBufferAddLit(&buf, " \"/dev/vhost-net\" rw,\n"); > > > - > > > if (needsVfio) { > > > virBufferAddLit(&buf, " \"/dev/vfio/vfio\" rw,\n"); > > > virBufferAddLit(&buf, " \"/dev/vfio/[0-9]*\" rw,\n"); > > > > A few things I noticed: > > > > - if I launch a vm with a virtio net device, then I can detach/attach as much > > as I want > > - if I launch a vm with one virtio net device, then detach it, /dev/vhost-net > > is not removed from the profile. > > - if launch a vm with one virtio net device, then I detach it and I shutdown > > the vm with no attached network devices, on the next vm start, the device is > > present and the profile still has the access. IIRC, this is by design > > - if I remove the net device from the vm definition then libvirtd updates > > profile to not allow the access. When I start the vm I cannot attach a virtio > > net device because the access is denied (this bug) > > - /dev/vhost-net is root:root 600 and vms typically run as non-root > > (libvirt-qemu:kvm here) and therefore a qemu process would not be able to > > open the device on its own. The only reason why qemu can is because libvirt > > passes an fd to it, and it is when qemu writes to it that the access is > > denied > > > > To me, it seems the real bug is that hotplug attach/detach is not updating the > > profile accordingly (attach adds it if it isn't there and detach removes it if > > no more net devices are present). Ideally, this would be the fix for this bug > > (we certainly support other hotplug/hotunplug events, like disks). > > Yes ideally it would, but I that would be a major change how hotplug > in general is handled by virt-aa-helper and I haven't had something > like that around easily. > I think we even talked about that quite a while a go when we also > talked about the inability to get more insight into e.g. pools from > the POV of virt-aa-helper. > It is on my "long term find no time for it list" :-/ > > > Considering > > that the device expands the attack surface to this part of the kernel and > > because the root:root 600 permissions show that non-root is not meant to access > > the device, this is most correct. > > > > That said, libvirt itself is providing protection here because it is mediating > > the access to /dev/vhost-net for the VMs since the VMs can't open the device > > themselves and they must rely on libvirt to pass in the fd. Because of this, > > adding the access to the profile is not providing additional protections beyond > > DAC *for this common use case and configuration*. Conditionally adding the > > access would provide benefit when 'user = "root"' is set in qemu.conf or the > > device itself has different permissions that allow the access (eg, 660 > > root:kvm). > > > > I maintain a preference for updating the profile on hotplug events. I'm willing > > to concede that adding the access for now until the correct solution is > > implemented would be acceptable, but I'd better like to understand how common > > it is to start a VM with no network devices and then hotplug one. > > I agree that if the use case is unreasonable we might declare this as > "work-as-intended" and the few people affected would then be supposed > to add a local override opting-in to open /dev/vhost up to all guests. > I'll ping on the bug for affected people to explain why/how they hit > the case, that might shed some light on it to allow us making the > decision if it is reasonable enough to concede the static rule as an > interim solution until one day it would be detected correctly on every > hotplug (and unplug) event. Hi, there was no further comment here so far. On the bug the following is what I got: (Note that it was reported to be triggered by Openstack usage) James Page: This is not the default otherwise no ones cloud would actually work right now. Instances are created with a network interface in paused state - at which point the interface plugging in neutron is executed; once that's completed the instance transitions to running and boots. That's the default behaviour with neutron/ml2/ovs driver. Are you using a different neutron driver? Daniel Pawlik (bug reporter): I also use neutron ml2 ovs driver. I understand that in default nova policy, only administrator can spawn instance without any interface, but if someone else can "tune" the policy, he/she will have a problem. No other feedback was added so far.

I personally tend to err on the side of caution and in this case would say an admin that wants to allow this can set up /dev/vhost-net as a local apparmor override. @Libvirt maintainers and @Jamie for apparmor guidance. Is in your opinion that enough of a common case that we'd add the the path as static rule in general? @JamesPage - you are likely the most experienced Openstack expert of us, what would be your opinion on the above? -- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd