[RFC] apparmor: add subprofile for virtiofsd

[Daniel P. Berrangé]

On Wed, Aug 26, 2020 at 10:41:15AM +0200, Christian Ehrhardt wrote:
> This is a continuation of
> https://www.redhat.com/archives/libvir-list/2020-August/msg00804.html
> https://www.redhat.com/archives/libvir-list/2020-August/msg00922.html
> 
> It still has too many weak points left, but should be great as an RFC
> already. virtiofsd works for me using that profile, but we need to:
> - agree on common paths to expect for virtiofsd

When you say  "common paths", I presume you're referring to paths
that users can export to the guests ?

If so, I fear that is an unsolvable problem if the goal is to define
a static policy ahead of time.  Apps can use pretty much arbitrary
dirs in their guest config.

We do have a /var/lib/libvirt/filesystems dir but I'm not convinced
any apps actually use it in practice.

Long term, I think we need an approach like the one we use for QEMU,
where we generate a dynamic polocy based on the guest config (apparmor),
or dynamic file lalbelling baed on guest config (selinux).

> - get the post pivot_root rules under control
> 
> ---
> 
> virtiofsd runs as root and is reachable from the guest, to limit
> the exploit potential this adds a apparmor subprofile to virtiofsd
> as spawned by libvirt to limit it.
> 
> Known TODOs:
> - rules after pivot_root need not to allow everything
> - settle on common paths with the community
> 
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt at canonical.com>
> ---
>  src/security/apparmor/libvirt-qemu         |  3 ++
>  src/security/apparmor/usr.sbin.libvirtd.in | 46 ++++++++++++++++++++++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/src/security/apparmor/libvirt-qemu b/src/security/apparmor/libvirt-qemu
> index a03e9e2c94..668fc72f27 100644
> --- a/src/security/apparmor/libvirt-qemu
> +++ b/src/security/apparmor/libvirt-qemu
> @@ -221,6 +221,9 @@
>    unix (send, receive) type=stream addr=none peer=(label=libvirtd),
>    unix (send, receive) type=stream addr=none peer=(label=/usr/sbin/libvirtd),
>  
> +  # allow to connect to virtiofsd
> +  unix (send, receive) type=stream addr=none peer=(label=libvirtd//virtiofsd),
> +
>    # for gathering information about available host resources
>    /sys/devices/system/cpu/ r,
>    /sys/devices/system/node/ r,
> diff --git a/src/security/apparmor/usr.sbin.libvirtd.in b/src/security/apparmor/usr.sbin.libvirtd.in
> index 4518e8f865..f878398b4b 100644
> --- a/src/security/apparmor/usr.sbin.libvirtd.in
> +++ b/src/security/apparmor/usr.sbin.libvirtd.in
> @@ -133,4 +133,50 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) {
>  
>     /usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper rmix,
>    }
> +
> +  # child profile for virtiofsd helper process
> +  /usr/{lib,lib64,lib/qemu,libexec}/virtiofsd Cx -> virtiofsd,
> +  profile virtiofsd flags=(attach_disconnected) {
> +   #include <abstractions/base>
> +   #include <abstractions/libvirt-qemu>
> +
> +   capability sys_admin,
> +   capability sys_resource,
> +
> +   # init phase
> +   / r,
> +   mount options=(rw, rslave)  -> /,
> +   umount /,
> +   mount options=(rw, nosuid, nodev, noexec, relatime)  -> @{PROC},
> +   owner /proc/sys/fs/file-max r,
> +
> +   # For communication/control from libvirtd
> +   unix (send, receive) type=stream addr=none peer=(label=libvirtd),
> +   signal (receive) set=("term") peer=/usr/sbin/libvirtd,
> +   signal (receive) set=("term") peer=libvirtd,
> +   owner /var/lib/libvirt/qemu/domain-*/fs[0-9]{[0-9],}-fs.pid w,
> +   /var/lib/libvirt/qemu/domain-*/fs[0-9]{[0-9],}-fs.sock rw,
> +   /var/lib/libvirt/qemu/ram/*/ram-node[0-9]{[0-9],} rw,
> +
> +   # For communication with confined and unconfined guests
> +   unix (send, receive) type=stream addr=none peer=(label=libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*),
> +   unix (send, receive) type=stream addr=none peer=(label=unconfined),
> +
> +   /usr/{lib,lib64,lib/qemu,libexec}/virtiofsd rmix,
> +
> +   # Common host paths to share from are allowed by default
> +   # Further paths should be added as local override
> +   # TODO - community 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,
> +
> +   # Site-specific additions and overrides. See local/README for details.
> +   #include <local/usr.lib.qemu.virtiofsd>
> +  }
> +
>  }
> -- 
> 2.28.0
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|