[libvirt] [PATCH] security: apparmor: make vhost-net access a static rule
Jamie Strandboge
jamie at canonical.com
Wed Apr 10 20:36:04 UTC 2019
On Wed, 20 Mar 2019, Christian Ehrhardt wrote:
> On Wed, Mar 20, 2019 at 8:45 AM Christian Ehrhardt
> <christian.ehrhardt at canonical.com> wrote:
> >
> > On Mon, Mar 4, 2019 at 11:42 AM Christian Ehrhardt
> > <christian.ehrhardt at canonical.com> wrote:
> > >
> > > On Fri, Mar 1, 2019 at 5:56 PM Jamie Strandboge <jamie at canonical.com> 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 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).
> > >
> > > 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.
>
> Today this was also added - quote here as FYI
> "The same behavior is reproduced in case when VM was created with network
> interface, interface detached, VM stopped (virsh destroy) and started
> again without network. This usecase does not need OpenStack admin
> privileges."
>
> > 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?
I maintain my preference as stated above for upstream libvirt since libvirt
supports other configurations besides the default of running as non-root.
Admins and distros are free to add the access as dictated by their
requirements.
I guess it would be conceivable to instead of addressing the real bug, checking
if libvirt is configured to run VMs as non-root and if so, adding the access.
This is really a band-aid on the problem and potentially confusing for users.
I'd much rather see that effort go towards fixing the hotplug/unplug issue.
--
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/20190410/9d20bca2/attachment-0001.sig>
More information about the libvir-list
mailing list