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

[Jamie Strandboge]

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 at canonical.com>
> ---
>  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). 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.

-- 
Jamie Strandboge             | http://www.canonical.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190301/7cb2f4e5/attachment-0001.sig>