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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Mar 4, 2019 at 11:42 AM Christian Ehrhardt
<christian.ehrhardt@xxxxxxxxxxxxx> wrote:
>
> On Fri, Mar 1, 2019 at 5:56 PM Jamie Strandboge <jamie@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > ---
> > >  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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux