When using [virtiofs], libvirtd must launch [virtiofsd] to provide
filesystem access on the host.  When a guest is configured with
virtiofs, such as:

    <filesystem type='mount' accessmode='passthrough'>
      <driver type='virtiofs'/>
      <source dir='/path'/>
      <target dir='mount_tag'/>
    </filesystem>

Attempting to start the guest fails with:

    internal error: virtiofsd died unexpectedly

/var/log/libvirt/qemu/$name-fs0-virtiofsd.log contains (as a single
line, wrapped below):

    libvirt:  error : cannot execute binary /usr/lib/qemu/virtiofsd:
    Permission denied

dmesg contains (as a single line, wrapped below):

    audit: type=1400 audit(1598229295.959:73): apparmor="DENIED"
    operation="exec" profile="libvirtd" name="/usr/lib/qemu/virtiofsd"
    pid=46007 comm="rpc-worker" requested_mask="x" denied_mask="x"
    fsuid=0 ouid=0

To avoid this, allow execution of virtiofsd from the libvirtd AppArmor
profile.

[virtiofs]: https://libvirt.org/kbase/virtiofs.html
[virtiofsd]: https://www.qemu.org/docs/master/interop/virtiofsd.html

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---

Changes in v2:
- Wrap log and dmesg messages, as requested by Christian Ehrhardt.

 src/security/apparmor/usr.sbin.libvirtd.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/security/apparmor/usr.sbin.libvirtd.in b/src/security/apparmor/usr.sbin.libvirtd.in
index 4518e8f865..f2030764cd 100644
--- a/src/security/apparmor/usr.sbin.libvirtd.in
+++ b/src/security/apparmor/usr.sbin.libvirtd.in
@@ -89,6 +89,7 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) {
   /usr/lib/xen-*/bin/libxl-save-helper PUx,
   /usr/lib/xen-*/bin/pygrub PUx,
   /usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx,
+  /usr/{lib,lib64,lib/qemu,libexec}/virtiofsd PUx,
 
   # Required by nwfilter_ebiptables_driver.c:ebiptablesWriteToTempFile() to
   # read and run an ebtables script.
-- 
2.28.0

On Tue, Aug 25, 2020 at 3:31 PM Kevin Locke <kevin@kevinlocke.name> wrote:
>
> When using [virtiofs], libvirtd must launch [virtiofsd] to provide
> filesystem access on the host.  When a guest is configured with
> virtiofs, such as:
>
>     <filesystem type='mount' accessmode='passthrough'>
>       <driver type='virtiofs'/>
>       <source dir='/path'/>
>       <target dir='mount_tag'/>
>     </filesystem>
>
> Attempting to start the guest fails with:
>
>     internal error: virtiofsd died unexpectedly
>
> /var/log/libvirt/qemu/$name-fs0-virtiofsd.log contains (as a single
> line, wrapped below):
>
>     libvirt:  error : cannot execute binary /usr/lib/qemu/virtiofsd:
>     Permission denied
>
> dmesg contains (as a single line, wrapped below):
>
>     audit: type=1400 audit(1598229295.959:73): apparmor="DENIED"
>     operation="exec" profile="libvirtd" name="/usr/lib/qemu/virtiofsd"
>     pid=46007 comm="rpc-worker" requested_mask="x" denied_mask="x"
>     fsuid=0 ouid=0
>
> To avoid this, allow execution of virtiofsd from the libvirtd AppArmor
> profile.
>
> [virtiofs]: https://libvirt.org/kbase/virtiofs.html
> [virtiofsd]: https://www.qemu.org/docs/master/interop/virtiofsd.html
>
> Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
> Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>

Thank you Kevin for the v2!
I've now also had the chance to test it and can confirm the reported issues
as well as the change fixing it.
With review and test in place I'll commit this apparmor change before
the 6.7.0 freeze happens.

But long term we should think about adding a profile for virtiofsd itself.
I have started some work but it is yet imperfect, it has open TODOs.
I'll reply with a RFC patch to this mail how that sub-profile could look
like and hope for a good discussion there from everyone.

In that RFC are questions for everyone (expected paths to agree on) as
well as apparmor specialists (I hope for Jamie) around pivot_root.

@Kevin - if you want you could continue your experiments with that
subprofile and let me know of the rough bumps that you find with it.

> ---
>
> Changes in v2:
> - Wrap log and dmesg messages, as requested by Christian Ehrhardt.
>
>  src/security/apparmor/usr.sbin.libvirtd.in | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/security/apparmor/usr.sbin.libvirtd.in b/src/security/apparmor/usr.sbin.libvirtd.in
> index 4518e8f865..f2030764cd 100644
> --- a/src/security/apparmor/usr.sbin.libvirtd.in
> +++ b/src/security/apparmor/usr.sbin.libvirtd.in
> @@ -89,6 +89,7 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) {
>    /usr/lib/xen-*/bin/libxl-save-helper PUx,
>    /usr/lib/xen-*/bin/pygrub PUx,
>    /usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx,
> +  /usr/{lib,lib64,lib/qemu,libexec}/virtiofsd PUx,
>
>    # Required by nwfilter_ebiptables_driver.c:ebiptablesWriteToTempFile() to
>    # read and run an ebtables script.
> --
> 2.28.0
>

--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

On Wed, 26 Aug 2020, Christian Ehrhardt wrote:

> On Tue, Aug 25, 2020 at 3:31 PM Kevin Locke <kevin@kevinlocke.name> wrote:
> >
> > When using [virtiofs], libvirtd must launch [virtiofsd] to provide
> > filesystem access on the host.  When a guest is configured with
> > virtiofs, such as:
> >
> >     <filesystem type='mount' accessmode='passthrough'>
> >       <driver type='virtiofs'/>
> >       <source dir='/path'/>
> >       <target dir='mount_tag'/>
> >     </filesystem>
> >
> > Attempting to start the guest fails with:
> >
> >     internal error: virtiofsd died unexpectedly
> >
> > /var/log/libvirt/qemu/$name-fs0-virtiofsd.log contains (as a single
> > line, wrapped below):
> >
> >     libvirt:  error : cannot execute binary /usr/lib/qemu/virtiofsd:
> >     Permission denied
> >
> > dmesg contains (as a single line, wrapped below):
> >
> >     audit: type=1400 audit(1598229295.959:73): apparmor="DENIED"
> >     operation="exec" profile="libvirtd" name="/usr/lib/qemu/virtiofsd"
> >     pid=46007 comm="rpc-worker" requested_mask="x" denied_mask="x"
> >     fsuid=0 ouid=0
> >
> > To avoid this, allow execution of virtiofsd from the libvirtd AppArmor
> > profile.

Adding the PUx rule for virtiofsd is in line with other helpers that
libvirt calls out to (eg, vhost-user-gpu, pygrub, etc). So +1 for the
below '/usr/{lib,lib64,lib/qemu,libexec}/virtiofsd PUx,' rule.

> > [virtiofs]: https://libvirt.org/kbase/virtiofs.html
> > [virtiofsd]: https://www.qemu.org/docs/master/interop/virtiofsd.html
> >
> > Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
> > Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> 
> Thank you Kevin for the v2!
> I've now also had the chance to test it and can confirm the reported issues
> as well as the change fixing it.
> With review and test in place I'll commit this apparmor change before
> the 6.7.0 freeze happens.
> 
> But long term we should think about adding a profile for virtiofsd itself.
> I have started some work but it is yet imperfect, it has open TODOs.
> I'll reply with a RFC patch to this mail how that sub-profile could look
> like and hope for a good discussion there from everyone.

I appreciate the sentiment on this point and think it is spot on to
consider to do this sort of thing for the helpers. With that said, IMO
we should keep in mind that the current libvirtd profile is wide open. I
thought I mentioned this on the mailing list, but couldn't find it, so
I'll describe why it is the way it is so we can have full context when
thinking through these sort of changes going forward.

Way back when the apparmor sVirt driver was introduced, there were
limitations with aa_change_profile() that required that the calling
process be running under a profile (other than the default 'unconfined'
profile), so we created the libvirtd profile, made it very lenient and
considered libvirtd a 'trusted helper' (since access to system:/// was
by design, intentionally very privileged). This is fine, libvirtd is
super flexible and requires a lot of privilege to do its job.

In other words, limiting libvirtd and getting in the way of the admin
wasn't a goal of the profile. The goal was to let libvirtd do what it
needed to do, which in addition to its existing design now included
loading per-VM security policy and launching VMs under that strict
policy to provide both guest to guest and guest to host protections.

Adding a form of privilege separation for the act of writing and loading
security policy would not get in the way of letting libvirtd do its job
and provided a meaningful security benefit as a hardening measure, so
rather than having libvirtd directly write out policy and load it we
decided instead to write virt-aa-helper to handle all the policy writing
and loading.  Sure, it would interpret domain xml that was under the
control of libvirtd, but this separation provides an extra hurdle and
the libvirtd profile doesn't allow directly writing a profile, loading
it, then transitioning to it. Since virt-aa-helper's job was discreet,
adding a child profile for it was straightforward and provided an
additional security benefit.

Since those early days, the aa_change_profile() limitation was lifted
and these days we could remove the libvirtd profile and transition its
qemu_bridge_helper child profile to a normal profile with binary
attachment like we do with virt-aa-helper. This would have similar
security properties as the existing policy.

I've not advocated for this for a couple of reasons:

1. adding child profiles to the libvirtd profile is convenient for us
   as an upstream because we don't have to worry about shipping another
   file on disk for any new helper policy that distributions have to
   concern themselves with
2. I like leaving the door open for improvements to libvirtd that would
   allow us to move the libvirtd profile to something more meaningfully
   strict
3. Leaving things as they are allows specialized builds of libvirt or
   administrators to easily patch the profile to be stricter for
   site-specific deployments. In other words, people don't have to
   start from scratch to limit via MAC what libvirtd is allowed to do
   in their specialized deployments.

As an upstream, '1' is compelling and enough of a reason to continue on
our path. As a user and distro packager, '3' is very nice to have.

With that in mind, when should we introduce child helper profiles? Like
with anything else, IMHO, we should consider the security benefit over
the usability and maintenance cost and ask ourselves questions like:

* is the helper processing untrusted input?
* is the helper doing privileged or otherwise sensitive operations?
* is the helper's task and required accesses discreet or open ended?
* does adding security policy block common use cases? Or put another
  way, does supporting common use cases significantly reduce the
  security benefits of the profile?
* are mechanisms in place to dynamically update the profile as driven by
  standard operations (eg, like when a user attaches a disk to a VM,
  virt-aa-helper adds the disk to the VM-specific profile)
* ...

IME, if we can't justify a security benefit relative to the usability
and maintenance costs, then a PUx rule to libvirtd's profile is
appropriate.

> In that RFC are questions for everyone (expected paths to agree on) as
> well as apparmor specialists (I hope for Jamie) around pivot_root.
> 
> @Kevin - if you want you could continue your experiments with that
> subprofile and let me know of the rough bumps that you find with it.

Christian and I spoke outside of this thread and I looked at his RFC
profile for this and these rules and notes can help us decide the
feasibility of a child profile:

   # Common host paths to share from are allowed by default
   # Further paths should be added as local override
   # TODO - community needs to settle on a list of common paths to allow
   owner /var/lib/libvirt/virtiofsd/*/ r,
   mount options=(rw, bind)  -> /var/lib/libvirt/virtiofsd/*/,
   pivot_root /var/lib/libvirt/virtiofsd/*/,

   # TODO - after pivot_root the rules for the actual file access by the guest
   # through virtiofsd would need to start with / which is too open
   /** rw,

AIUI, the virtiofs driver allows for the domain XML to specify a path on
the host to be exposed to the guest in a manner that the guest can mount
path in the guest. We can apply our criteria to Christian's exploratory
rules:

* is the helper processing untrusted input?
  - No. Per the documentation this is driven by the admin who has access
    to the libvirt socket (ie, someone granted this level of access to
    libvirtd owns the system)
* is the helper doing privileged or otherwise sensitive operations?
  - Yes, it is exposing arbitrary files from the host to the guest
* is the helper's task and required accesses discreet or open ended?
  - The task is discreet but the accesses are open ended
* does adding security policy block common use cases? Or put another
  way, does supporting common use cases significantly reduce the
  security benefits of the profile?
  - the first 3 of Christian's exploratory rules show he had to make a
    compromise to limit what virtiofsd would expose to guests to have a
    meaningful security benefit. This would impact usability
  - the '/** rw,' access is an unfortunate limitation of post-pivot_root
    accesses and current AppArmor in the kernel due to AppArmor not
    having the required context at the time of mediation (work is
    ongoing to address this)
* are mechanisms in place to dynamically update the profile as driven by
  standard operations (eg, like when a user attaches a disk to a VM,
  virt-aa-helper adds the disk to the VM-specific profile)
  - this is not in place now, but in theory could be to assist with the
    pivot_root limitation (eg, we could have a virtiofsd child profile
    that includes a directory managed by virt-aa-helper(/something else)
    that has per-VM rules for the paths specified in those VM's domain
    XML)

Considering all of the above, IME a PUx rule is the right choice. A
security audit of virtiofsd IMO is warranted to ensure a non-root user
who doesn't have access to libvirtd's socket can't start a VM with
session:/// (etc) to expose a privileged file like /etc/shadow from the
host to the guest (and thus circumventing host protections on
/etc/shadow). Considering this last point, there is arguably value in
dynamically updating the virtiofsd child profile. This development cost
should be weighed that against future improvements to AppArmor to
address the post-pivot_root limitation.

Hope this helps!

PS - in writing my response I noticed that we are allowing 'w' for paths
that have x rules. We probably at a minimum should have audit deny rules
for at least helpers that have profiles. Eg:

  audit deny /usr/{lib,lib64}/libvirt/virt-aa-helper w,
  audit deny /usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper w,

(I consider this a hardening measure and not a security bug since the
libvirtd profile is intentionally lenient).


> > ---
> >
> > Changes in v2:
> > - Wrap log and dmesg messages, as requested by Christian Ehrhardt.
> >
> >  src/security/apparmor/usr.sbin.libvirtd.in | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/src/security/apparmor/usr.sbin.libvirtd.in b/src/security/apparmor/usr.sbin.libvirtd.in
> > index 4518e8f865..f2030764cd 100644
> > --- a/src/security/apparmor/usr.sbin.libvirtd.in
> > +++ b/src/security/apparmor/usr.sbin.libvirtd.in
> > @@ -89,6 +89,7 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) {
> >    /usr/lib/xen-*/bin/libxl-save-helper PUx,
> >    /usr/lib/xen-*/bin/pygrub PUx,
> >    /usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx,
> > +  /usr/{lib,lib64,lib/qemu,libexec}/virtiofsd PUx,
> >
> >    # Required by nwfilter_ebiptables_driver.c:ebiptablesWriteToTempFile() to
> >    # read and run an ebtables script.
> > --
> > 2.28.0
> >
> 
> --
> Christian Ehrhardt
> Staff Engineer, Ubuntu Server
> Canonical Ltd
> 
-- 
Jamie Strandboge             | http://www.canonical.com

Hi Christian and Jamie,

On Wed, 2020-08-26 at 09:26 -0500, Jamie Strandboge wrote:
> [...]
>
> Considering all of the above, IME a PUx rule is the right choice. A
> security audit of virtiofsd IMO is warranted to ensure a non-root user
> who doesn't have access to libvirtd's socket can't start a VM with
> session:/// (etc) to expose a privileged file like /etc/shadow from the
> host to the guest (and thus circumventing host protections on
> /etc/shadow). Considering this last point, there is arguably value in
> dynamically updating the virtiofsd child profile. This development cost
> should be weighed that against future improvements to AppArmor to
> address the post-pivot_root limitation.

I don't have enough AppArmor or virtiofsd expertise to weigh in on the
cost/benefit trade-offs or implementation details, but just wanted to
commend both your effort on the RFC and depth of this response.  I think
the motivations are sound and I agree on all counts.  I'd be happy to
help where I can with any approach.

As a fairly unsophisticated user, I'd also warn against further
complicating virtio-fs setup.  I found the current process (which
requires configuring vNUMA with shared memory backing in addition to
hand-editing <filesystem>) difficult and error-prone.  If editing
AppArmor configuration files for each shared directory, and keeping them
in sync with the domain XML will be required, I worry that it will add
significant burden and risk of error for other unsophisticated users.
If the AppArmor child policy can be dynamically updated as Jamie
suggested (IIUC) I wouldn't expect this to be an issue.

Thanks for considering,
Kevin