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

Christian Ehrhardt posted 1 patch 5 years, 1 month ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190218171259.5916-1-christian.ehrhardt@canonical.com
src/security/apparmor/libvirt-qemu |  1 +
src/security/virt-aa-helper.c      | 17 +----------------
2 files changed, 2 insertions(+), 16 deletions(-)
[libvirt] [PATCH] security: apparmor: make vhost-net access a static rule
Posted by Christian Ehrhardt 5 years, 1 month ago
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@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");
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] security: apparmor: make vhost-net access a static rule
Posted by Jamie Strandboge 5 years ago
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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] security: apparmor: make vhost-net access a static rule
Posted by Christian Ehrhardt 5 years ago
On Fri, Mar 1, 2019 at 5:56 PM Jamie Strandboge <jamie@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@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.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] security: apparmor: make vhost-net access a static rule
Posted by Christian Ehrhardt 5 years ago
On Mon, Mar 4, 2019 at 11:42 AM Christian Ehrhardt
<christian.ehrhardt@canonical.com> wrote:
>
> On Fri, Mar 1, 2019 at 5:56 PM Jamie Strandboge <jamie@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@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.
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] security: apparmor: make vhost-net access a static rule
Posted by Christian Ehrhardt 5 years ago
On Wed, Mar 20, 2019 at 8:45 AM Christian Ehrhardt
<christian.ehrhardt@canonical.com> wrote:
>
> On Mon, Mar 4, 2019 at 11:42 AM Christian Ehrhardt
> <christian.ehrhardt@canonical.com> wrote:
> >
> > On Fri, Mar 1, 2019 at 5:56 PM Jamie Strandboge <jamie@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@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?
>
>
>
> --
> Christian Ehrhardt
> Software Engineer, Ubuntu Server
> Canonical Ltd



-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] security: apparmor: make vhost-net access a static rule
Posted by Jamie Strandboge 4 years, 11 months ago
On Wed, 20 Mar 2019, Christian Ehrhardt wrote:

> On Wed, Mar 20, 2019 at 8:45 AM Christian Ehrhardt
> <christian.ehrhardt@canonical.com> wrote:
> >
> > On Mon, Mar 4, 2019 at 11:42 AM Christian Ehrhardt
> > <christian.ehrhardt@canonical.com> wrote:
> > >
> > > On Fri, Mar 1, 2019 at 5:56 PM Jamie Strandboge <jamie@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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list