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 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