[libvirt] [PATCH 3/4] apparmor: allow expected /tmp access patterns

[Christian Ehrhardt]

On Mon, Aug 13, 2018 at 7:08 PM Jamie Strandboge <jamie at canonical.com>
wrote:

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

I updated the comments and commit message to better cover the existing
concerns and TODOs.
And that this is a tradeoff between usability and security.

Further I split the general /tmp from the smb access, to discuss those two
changes individually as needed.

Thanks for the feedback on this!


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



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