On Mon, Aug 13, 2018 at 7:08 PM Jamie Strandboge <ja...@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.ehrha...@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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list