On Tue, 2018-08-14 at 08:18 +0200, Christian Ehrhardt wrote: > The samba feature of qemu will place the samba config file in > /tmp/qemu-smb.<PID>. > > But at least it has a predictable path identifying qemu-smb feature > itself by an infix in the path. > > This is a compromise of security and usability as the "owner" > restriction > will not protect guests among each other. > Therefore the rule added makes the feature usable, but does not allow > cross guest protection. > > Core issue is, that it is currently impossible to predict the PID > which would > follow "qemu-smb-", but long term, once the samba feature would be > exposed in > guest XML we'd prefer a virt-aa-helper based solution that can render > the > samba rule on demand and with a custom PID into the per guest > profile. > > But the same is true for manual user overrides for this feature as > well, > they can neither predict the PID, nor have a local include per-guest. > Thereby > punting this to the user to add the rule later will not make it > safer, but > only less usable. While the facts are true, there is something omitted and I come to a different conclusion. It is true that the pid is unpredictable, but with the 'owner /{,var/}tmp/**/ r,' rule, it is easy for a confined process to find the directories. Also, the rule 'owner /tmp/qemu- smb.*/{,**} rw,' (below) allows writing /tmp/qemu-smb.*/ so symlink attacks are possible by a rogue VM against other VMs. Rogue VMs could also use the directory in coordinated file sharing (but I mention this only for completeness, not as an objection, since there are other rules in the policy that 'overlap' and allow this sort of thing). It is definitely a usability improvement to include the rule, but I disagree that it isn't safer without it-- your addition applies to all libvirt/apparmor users, many of whom will not use the smb feature that isn't supported by the domain xml. Now, you could argue that users that never use the smb feature aren't affected by the proposed global rule (which is true), but omitting this patch, users can add the glob rule to the per-VM site policy in /etc/apparmor.d/libvirt/libvirt-<uuid> for only the VMs they need it. This might be useful if they, for example, have one trusted VM that uses the smb feature and other untrusted VMs that do not. With the global rule, the untrusted VMs have access by default. These concerns are somewhat contrived and I'm ambivalent about the addition, but it bothers me that we are adding a rule for a use case that isn't directly supported by libvirt. +0 as is.

If you can demonstrate that qemu guards against symlink attacks on the dir and is otherwise safe from attack (eg, open(..., O_CREAT|O_EXCL) followed by unlink(...)), etc, etc then I could be persuaded to give a +1.

> Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; > --- > examples/apparmor/libvirt-qemu | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/examples/apparmor/libvirt-qemu > b/examples/apparmor/libvirt-qemu > index 6971d3db03..350b13b824 100644 > --- a/examples/apparmor/libvirt-qemu > +++ b/examples/apparmor/libvirt-qemu > @@ -191,6 +191,11 @@ > # want more unique paths per rule. > /{,var/}tmp/ r, > owner /{,var/}tmp/**/ r, > + # allow qemu smb feature specific path with write access > + # TODO: This is a compromise between security and usability - once > e.g. samba > + # would be expressed in libvirt XML it should be added on demand > via > + # virt-aa-helper instead. > + owner /tmp/qemu-smb.*/{,**} rw, > > # for file-posix getting limits since 9103f1ce > /sys/devices/**/block/*/queue/max_segments r, -- Jamie Strandboge | http://www.canonical.com

Libvirt now tries to preserve all mounts under /dev in qemu namespaces. The old rules only listed a set of known paths but those are no more enough. I found some due to containers like /dev/.lxc/* and such but also /dev/console and /dev/net/tun. Libvirt is correct to do so, but we can no more predict the names properly, so we modify the rule to allow a wildcard based pattern matching what libvirt does. Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; --- examples/apparmor/usr.sbin.libvirtd | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/examples/apparmor/usr.sbin.libvirtd b/examples/apparmor/usr.sbin.libvirtd index 3ff43c32a2..8625862ced 100644 --- a/examples/apparmor/usr.sbin.libvirtd +++ b/examples/apparmor/usr.sbin.libvirtd @@ -33,17 +33,11 @@ mount options=(rw,rslave) -> /, mount options=(rw, nosuid) -> /{var/,}run/libvirt/qemu/*.dev/, - mount options=(rw, move) /dev/ -> /{var/,}run/libvirt/qemu/*.dev/, - mount options=(rw, move) /dev/hugepages/ -> /{var/,}run/libvirt/qemu/*.hugepages/, - mount options=(rw, move) /dev/mqueue/ -> /{var/,}run/libvirt/qemu/*.mqueue/, - mount options=(rw, move) /dev/pts/ -> /{var/,}run/libvirt/qemu/*.pts/, - mount options=(rw, move) /dev/shm/ -> /{var/,}run/libvirt/qemu/*.shm/, - - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.dev/ -> /dev/, - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.hugepages/ -> /dev/hugepages/, - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.mqueue/ -> /dev/mqueue/, - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.pts/ -> /dev/pts/, - mount options=(rw, move) /{var/,}run/libvirt/qemu/*.shm/ -> /dev/shm/, + # libvirt provides any mounts under /dev to qemu namespaces + mount options=(rw, move) /dev/ -> /{var/,}run/libvirt/qemu/*.dev/, + mount options=(rw, move) /dev/** -> /{var/,}run/libvirt/qemu/*{/,}, + mount options=(rw, move) /{var/,}run/libvirt/qemu/*.dev/ -> /dev/, + mount options=(rw, move) /{var/,}run/libvirt/qemu/*{/,} -> /dev/**,