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@canonical.com>
> ---
>  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