<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Aug 13, 2018 at 7:08 PM Jamie Strandboge <<a href="mailto:jamie@canonical.com">jamie@canonical.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, 2018-08-13 at 16:39 +0200, Christian Ehrhardt wrote:<br>
> Several cases were found needing /tmp, for example ceph will try to<br>
> list /tmp<br>
> and the samba feature of qemu will place things in /tmp/qemu-smb.*.<br>
> This is sort of safe because:<br>
>  - While /tmp could contain anything it is not recommended to put<br>
> critical<br>
>    data there anyway<br>
>  - We restrict general access to only dir listing and reading of<br>
> files owned<br>
>    (intentionally not the full power of user-tmp abstraction)<br>
>  - While it would be hard to predict the PID as part of the string<br>
> for the<br>
>    qemu smb feature (this is not exposed through XML so virt-aa-<br>
> helper<br>
>    can't help) it is guarded by the "owner" statement and a pretty<br>
> clear<br>
>    qemu-smb infix in the path.<br>
> <br>
> Signed-off-by: Christian Ehrhardt <<a href="mailto:christian.ehrhardt@canonical.com" target="_blank">christian.ehrhardt@canonical.com</a>><br>
> ---<br>
>  examples/apparmor/libvirt-qemu | 10 ++++++++++<br>
>  1 file changed, 10 insertions(+)<br>
> <br>
> diff --git a/examples/apparmor/libvirt-qemu<br>
> b/examples/apparmor/libvirt-qemu<br>
> index 5caf14e418..c4f231b328 100644<br>
> --- a/examples/apparmor/libvirt-qemu<br>
> +++ b/examples/apparmor/libvirt-qemu<br>
> @@ -180,6 +180,16 @@<br>
>    # for rbd<br>
>    /etc/ceph/ceph.conf r,<br>
>  <br>
> +  # various functions will need /tmp (e.g. ceph), allow the base dir<br>
> and a<br>
> +  # few known functions.<br>
> +  # we want to avoid to give blanket read or even write to<br>
> everything under /tmp<br>
> +  # so users are expected to add site specific addons for more<br>
> uncommon cases.<br>
> +  # allow only dir listing and owner based file read<br>
> +  /{,var/}tmp/ r,<br>
> +  owner /{,var/}tmp/**/ r,<br>
> +  # allow qemu smb feature specific path with write access<br>
> +  owner /tmp/qemu-smb.*/{,**} rw,<br>
<br>
The actual rule additions are a compromise between security and<br>
usability. Personally I'd prefer they not be there and let admins add<br>
them or allow distros to patch them. That said, the comments and commit<br>
message don't seem quite right for what you are adding. Eg, you mention<br>
ceph, but there is no ceph rule and the profile comment doesn't mention<br>
ceph only needs to list dirs; the profile comments are formatted a bit<br>
weird too.<br>
<br>
You mention 'sort of safe', but because of the '/{,var/}tmp/ r,' rule,<br>
you are allowing guests to enumerate all the /tmp/qemu-smb.*<br>
directories and then there is a 'rw' rule for that path. While this is<br>
an 'owner' match, in practice, VMs all run under the same uid so this<br>
means a rogue vm would be allowed to access files in these directories.<br>
That said, I don't know what qemu is setting up in there-- so maybe it<br>
is designed in such a way that this doesn't matter.<br>
<br>
I'd much rather not call this 'sort of safe' but instead call out the<br>
problem, justify why the rule should be there and perhaps add a TODO<br>
that once smb is supported in domain xml that this rule will be added<br>
conditionally.<br></blockquote><div><br></div><div>I updated the comments and commit message to better cover the existing concerns and TODOs.</div><div>And that this is a tradeoff between usability and security.</div><div><br></div><div>Further I split the general /tmp from the smb access, to discuss those two changes individually as needed.</div><div><br></div><div>Thanks for the feedback on this!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+0 for rule inclusion provided the comments and commit message are<br>
addressed. +1 if it can be demonstrated that qemu is handling those<br>
dirs safely wrt vm isolation.<br>
<br>
-- <br>
Jamie Strandboge             | <a href="http://www.canonical.com" rel="noreferrer" target="_blank">http://www.canonical.com</a></blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><span style="color:rgb(136,136,136);font-size:12.8px">Christian Ehrhardt</span><div style="color:rgb(136,136,136);font-size:12.8px">Software Engineer, Ubuntu Server</div><div style="color:rgb(136,136,136);font-size:12.8px">Canonical Ltd</div></div></div></div></div></div>