[virt-tools-list] [PATCH] Don't create disk images world readable and executable

[Ron]

On Wed, Jul 02, 2014 at 09:02:26AM +0200, Martin Kletzander wrote:
> On Tue, Jul 01, 2014 at 11:57:11PM +0930, Ron wrote:
> >
> >The other reason to set the mode explicitly rather then changing
> >the umask in code is that this would seem to be error prone for
> >programmers, based on what I'm seeing in libvirt which does it
> >that way.  That's a separate bug report/patch for the other list,
> >but the context relevant here is it does something like:
> >
> >int virFileMakePath(const char *path)
> >{
> >   return virFileMakePathWithMode(path, 0777);
> >}
> >
> >...
> >old_umask = umask(077); virFileMakePath(rundir); umask(old_umask);
> 
> The only places I see it used like that is in daemons before any
> threads are created (which is not an excuse, of course) and hostdev
> manager (this might be an issue, although it gets called on driver
> initialization only).

Yeah, I haven't audited this in great detail yet, since it touches
lots of places, so I figured the first step with that one was to
get some in principle agreement that cleaning this up would be a
Good Idea, and confirm there wasn't some other rationale that I
was missing.

> Such code can, however, of course call virFileMakePathWithMode(path,
> 0700) instead.

Right, aside from having to do this in quite a few places, it should
be a fairly straightforward fix.  The WithMode() function passes the
mode directly to mkdir(2) which is thread-safe, and as a mandatory
argument it forces people to think about what the correct mode should
be (and makes it clear in the code what mode was intended in each
case for each directory created).

>That is unless I overlooked the need of removing the first '7' for
> user permission mask.

We do need the 'executable' bit set for directories, since that
actually means "make this directory searchable" for those.


> >Which aside from being racy if virFileMakePath() is called from
> >multiple threads, means we currently end up with directory trees
> >like:
> >
> ># ls -lR /var/cache/libvirt/
> >/var/cache/libvirt/:
> >drwxr-x--- 4 libvirt-qemu libvirt-qemu 4096 Jun  7 05:01 qemu
> 
> I don't experience this on my system, are you sure this is created by
> the daemon?  It should be created by the installation (according to
> src/Makefile.am and libvirt.spec.in), but as you said, that's another
> bug/patch and should be dealt with in libvir-list.

Hmm.  I have 3 systems here on libvirt 1.2.4, and they all do the same
thing. And if I look at qemu_capabilities.c: virQEMUCapsRememberCached()
the code calls virFileMakePath(capsdir), without doing the umask trick.

So unless I'm missing it being set somewhere else (there's no umask
calls in that file at all), I'm not sure why you might see something
different for this dir:

> >/var/cache/libvirt/qemu:
> >drwxr-xr-x 2 root root 4096 Jun 30 16:22 capabilities

But yes, let's take this discussion to libvir-list@ once the 1.2.6
deadline goes whoosh :)



> >I think the main questions I have for the patch I sent here are:
> >
> >- is 640 really the most appropriate for each of those cases?
> 
> Yes, no executable should be created by the lines you've modified (and
> I see those are all using O_CREAT, great), neither there should be any
> need for some groups to write into them.  Using 0600 would be too
> much, I think.

My main uncertainty here is the number of possible permutations we
currently have for which users and groups might actually need to
have access to this.

On Debian at least, libvirt is built with:
 --with-qemu-{user,group}=libvirt-qemu

And non-root users are granted access to the system instance by
being added to group libvirt.

Which means 640 or 660 perms with owner libvirt-qemu:libvirt
generally does the right thing, qemu has read/write permission,
and people in the libvirt group have read-only permission to
do things like make backup copies, and can safely be given write
permission too if that is needed.

But in order to make that work properly with the disk images, I
needed to disable dynamic_ownership, otherwise when the VM gets
shut down, libvirt changes the owner to root:root, and effectively
locks both of them out ...

That could probably be fixed by implementing the XXX in
security_dac.c: virSecurityDACRestoreSecurityFileLabel(), to
actually restore the real previous owner rather than just
blindly setting it to 0:0 (root:root).

So it might be possible there are some other cases that only
previously worked because this was world readable too which will
shake out now.  But this is what we ultimately want, so if/when
they do, we should fix them wherever they might need it too :)



> >- should that be a configuration option rather than hard coded?
> 
> I see no reason for having more lax permissions as 640 and stricter
> permissions can be modified by umask as said before.

Using 0660 might be a reasonable choice for some users in some
cases too (such as if you wanted people in the libvirt group to
be able to run virt-sparsify on offline images or something like
that).  But I'm still building up my own use cases and patterns
right now, so I don't have a deep insight into what others might
be doing yet ...

> >but the basic principle of deciding that directly for each call
> >to open(), as POSIX requires, is the right direction to go.
> 
> Yes, once again sorry for the confusion, ACK to this patch and I'm
> applying it in a minute.
> 
> Congrats to your first patch in virt-manager.

That's perfectly ok.  Good code review is all about making sure
that all of the open questions anyone has have satisfactory
answers.

Thanks for the prompt review :)  That certainly makes it much
easier to try to find more time to tackle the other things I'd
like to see this do well which still need a bit of work too!

  Best,
  Ron