On Mon, 2018-08-13 at 16:39 +0200, Christian Ehrhardt wrote:
> Several cases were found needing /tmp, for example ceph will try to
> list /tmp
> and the samba feature of qemu will place things in /tmp/qemu-smb.*.
> This is sort of safe because:
>  - While /tmp could contain anything it is not recommended to put
> critical
>    data there anyway
>  - We restrict general access to only dir listing and reading of
> files owned
>    (intentionally not the full power of user-tmp abstraction)
>  - While it would be hard to predict the PID as part of the string
> for the
>    qemu smb feature (this is not exposed through XML so virt-aa-
> helper
>    can't help) it is guarded by the "owner" statement and a pretty
> clear
>    qemu-smb infix in the path.
>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  examples/apparmor/libvirt-qemu | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/examples/apparmor/libvirt-qemu
> b/examples/apparmor/libvirt-qemu
> index 5caf14e418..c4f231b328 100644
> --- a/examples/apparmor/libvirt-qemu
> +++ b/examples/apparmor/libvirt-qemu
> @@ -180,6 +180,16 @@
>    # for rbd
>    /etc/ceph/ceph.conf r,
> 
> +  # various functions will need /tmp (e.g. ceph), allow the base dir
> and a
> +  # few known functions.
> +  # we want to avoid to give blanket read or even write to
> everything under /tmp
> +  # so users are expected to add site specific addons for more
> uncommon cases.
> +  # allow only dir listing and owner based file read
> +  /{,var/}tmp/ r,
> +  owner /{,var/}tmp/**/ r,
> +  # allow qemu smb feature specific path with write access
> +  owner /tmp/qemu-smb.*/{,**} rw,

The actual rule additions are a compromise between security and
usability. Personally I'd prefer they not be there and let admins add
them or allow distros to patch them. That said, the comments and commit
message don't seem quite right for what you are adding. Eg, you mention
ceph, but there is no ceph rule and the profile comment doesn't mention
ceph only needs to list dirs; the profile comments are formatted a bit
weird too.

You mention 'sort of safe', but because of the '/{,var/}tmp/ r,' rule,
you are allowing guests to enumerate all the /tmp/qemu-smb.*
directories and then there is a 'rw' rule for that path. While this is
an 'owner' match, in practice, VMs all run under the same uid so this
means a rogue vm would be allowed to access files in these directories.
That said, I don't know what qemu is setting up in there-- so maybe it
is designed in such a way that this doesn't matter.

I'd much rather not call this 'sort of safe' but instead call out the
problem, justify why the rule should be there and perhaps add a TODO
that once smb is supported in domain xml that this rule will be added
conditionally.

+0 for rule inclusion provided the comments and commit message are
addressed. +1 if it can be demonstrated that qemu is handling those
dirs safely wrt vm isolation.

--
Jamie Strandboge             | http://www.canonical.com