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

[Ron]

On Tue, Jul 01, 2014 at 10:01:17AM +0200, Martin Kletzander wrote:
> On Tue, Jul 01, 2014 at 03:36:54AM +0930, Ron wrote:
> >On Mon, Jun 30, 2014 at 02:39:37PM +0200, Martin Kletzander wrote:
> >>On Sun, Jun 29, 2014 at 04:16:36PM +0930, Ron wrote:
> >>>Python's os.open() defaults to mode 0777 if not explicitly specified.
> >>
> >>Not really, or at least not on my system.  That must be some umask or
> >>fs issue or something:
> >
> >No, it's still modified by the umask that is active at the time,
> >so your example below is what you see if the umask is 022.  Sorry
> >if I wasn't clear on that, but the umask is indeed the only thing
> >keeping it from being 777 otherwise.
> >
> 
> I wasn't clear on that either, sorry.  It should definitely obey the
> umask set on that process.  But what I was worried about was that
> there is umask (or fmask) set to 000.  I just wanted to ask if that
> shouldn't be right thing to solve; that way we don't have to deal with
> all open() calls, but rather solve it only on one place.

The POSIX open(2) function specifies that passing the mode parameter
is required when O_CREAT (or on Linux O_TMPFILE) flags are used.

It seems that python chose to make that optional, and defaults to
"be as permissive as possible" if it's not specified.

I think there's basically 2 principles we should be applying here:

 - respect the caller's umask.  Since a) they set that themselves to
   define the desired policy for their environment, so we shouldn't
   be changing it out from underneath them.  And b) the umask is
   global so changing it, even temporarily, is not thread-safe, and
   could have unexpected consequences for anything else created in
   the time we do that (or for us if something else changes it once
   again before our creation tasks are done).

 - explicitly specify the maximum permission that is appropriate for
   each file we create - possibly configurable by some user --option
   if needed, since there may not be a one-size-fits-all answer for
   every file.  Disk images shouldn't be world readable or executable,
   but maybe something else we create (now or later) should or can be.

So I think the right thing to do is to *never* touch the umask we
inherited, that defines the disallowed permissions that the caller's
environment desires for even the least sensitive files - and to
always specify what the maximum safe/needed permission is for each
individual file that we create, when we create them, since we have
some idea of whether they are likely to contain 'highly' sensitive
data or not.

Users can always make them more permissive later if they want to,
but they can't go the other way to make them less permissive without
a 'race' (of possibly many minutes here) that leaves them exposed.



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);


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

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

 /var/cache/libvirt/qemu/capabilities:
 -rw------- 1 root root 6065 Jun 30 16:22 2d1567fa77de202a8d45125ec58eadc9538916e0d21b9ecbc092de24d27ecf9c.xml


Where it would seem that the code forgot to do the umask trick
before creating the capabilities directory and left it being
0777 & ~callers_umask.

The patch I'd like to propose for discussion there is to replace
all uses of virFileMakePath() with calls to virFileMakePathWithMode
directly (since it is thread safe for setting the mode), and get
rid of the unsafe / easy to forget, umask dance everywhere it is
used.  (but since it sounds like they are close to releasing 1.2.6
that might have to wait until the window for 1.2.7 ...).



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?
 - should that be a configuration option rather than hard coded?

but the basic principle of deciding that directly for each call
to open(), as POSIX requires, is the right direction to go.

  Cheers,
  Ron