<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Aug 15, 2018 at 7:35 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 Tue, 2018-08-14 at 08:18 +0200, Christian Ehrhardt wrote:<br>
> The samba feature of qemu will place the samba config file in<br>
> /tmp/qemu-smb.<PID>.<br>
> <br>
> But at least it has a predictable path identifying qemu-smb feature<br>
> itself by an infix in the path.<br>
> <br>
> This is a compromise of security and usability as the "owner"<br>
> restriction<br>
> will not protect guests among each other.<br>
> Therefore the rule added makes the feature usable, but does not allow<br>
> cross guest protection.<br>
> <br>
> Core issue is, that it is currently impossible to predict the PID<br>
> which would<br>
> follow "qemu-smb-", but long term, once the samba feature would be<br>
> exposed in<br>
> guest XML we'd prefer a virt-aa-helper based solution that can render<br>
> the<br>
> samba rule on demand and with a custom PID into the per guest<br>
> profile.<br>
> <br>
> But the same is true for manual user overrides for this feature as<br>
> well,<br>
> they can neither predict the PID, nor have a local include per-guest. <br>
> Thereby<br>
> punting this to the user to add the rule later will not make it<br>
> safer, but<br>
> only less usable.<br>
<br>
While the facts are true, there is something omitted and I come to a<br>
different conclusion. It is true that the pid is unpredictable, but<br>
with the 'owner /{,var/}tmp/**/ r,' rule, it is easy for a confined<br>
process to find the directories. Also, the rule 'owner /tmp/qemu-<br>
smb.*/{,**} rw,' (below) allows writing /tmp/qemu-smb.*/ so symlink<br>
attacks are possible by a rogue VM against other VMs. Rogue VMs could<br>
also use the directory in coordinated file sharing (but I mention this<br>
only for completeness, not as an objection, since there are other rules<br>
in the policy that 'overlap' and allow this sort of thing).<br>
<br>
It is definitely a usability improvement to include the rule, but I<br>
disagree that it isn't safer without it-- your addition applies to all<br>
libvirt/apparmor users, many of whom will not use the smb feature that<br>
isn't supported by the domain xml. Now, you could argue that users that<br>
never use the smb feature aren't affected by the proposed global rule<br>
(which is true), but omitting this patch, users can add the glob rule<br>
to the per-VM site policy in /etc/apparmor.d/libvirt/libvirt-<uuid> for<br>
only the VMs they need it. This might be useful if they, for example,<br>
have one trusted VM that uses the smb feature and other untrusted VMs<br>
that do not. With the global rule, the untrusted VMs have access by<br>
default.<br>
<br>
These concerns are somewhat contrived and I'm ambivalent about the<br>
addition, but it bothers me that we are adding a rule for a use case<br>
that isn't directly supported by libvirt. +0 as is.<br></blockquote><div><br></div><div>Thanks for laying out the security concerns on this in detail.</div><div>I agree, this might be too much for a general rule and has to stay out of the abstraction.</div><div><br></div><div>We'd have to wait for the feature to be supported by libvirt to fully support it.</div><div>Even then we might not have the pid at the time the rule is created since qemu was not yet spawned at that point.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
If you can demonstrate that qemu guards against symlink attacks on the<br>
dir and is otherwise safe from attack (eg, open(..., O_CREAT|O_EXCL)<br>
followed by unlink(...)), etc, etc then I could be persuaded to give a<br>
+1.<br></blockquote><div><br></div><div>I can't, therefore I'll abandon this change for now.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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 | 5 +++++<br>
>  1 file changed, 5 insertions(+)<br>
> <br>
> diff --git a/examples/apparmor/libvirt-qemu<br>
> b/examples/apparmor/libvirt-qemu<br>
> index 6971d3db03..350b13b824 100644<br>
> --- a/examples/apparmor/libvirt-qemu<br>
> +++ b/examples/apparmor/libvirt-qemu<br>
> @@ -191,6 +191,11 @@<br>
>    # want more unique paths per rule.<br>
>    /{,var/}tmp/ r,<br>
>    owner /{,var/}tmp/**/ r,<br>
> +  # allow qemu smb feature specific path with write access<br>
> +  # TODO: This is a compromise between security and usability - once<br>
> e.g. samba<br>
> +  # would be expressed in libvirt XML it should be added on demand<br>
> via<br>
> +  # virt-aa-helper instead.<br>
> +  owner /tmp/qemu-smb.*/{,**} rw,<br>
>  <br>
>    # for file-posix getting limits since 9103f1ce<br>
>    /sys/devices/**/block/*/queue/max_segments r,<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>