[libvirt] [PATCH v2 4/5] apparmor: allow qemu-smb access in /tmp

[Christian Ehrhardt]

On Wed, Aug 15, 2018 at 7:35 PM Jamie Strandboge <jamie at canonical.com>
wrote:

> 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.
>

Thanks for laying out the security concerns on this in detail.
I agree, this might be too much for a general rule and has to stay out of
the abstraction.

We'd have to wait for the feature to be supported by libvirt to fully
support it.
Even then we might not have the pid at the time the rule is created since
qemu was not yet spawned at that point.


> 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.
>

I can't, therefore I'll abandon this change for now.


>
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt at 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



-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180816/d0d3813c/attachment-0001.htm>