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

Wed Jul 2 07:02:26 UTC 2014

On Tue, Jul 01, 2014 at 11:57:11PM +0930, Ron wrote:
>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.

Definitely, what I meant was just further restricting what was set...

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

...and I haven't realized umask is per-process.

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

And that's exactly what you've proposed and what I agree with, sorry
for doubting it.  I somehow saw a different problem in the first mail,
then what you were trying to fix and got caught in it.

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

Definitely.

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

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

Such code can, however, of course call virFileMakePathWithMode(path,
0700) instead.  That is unless I overlooked the need of removing the
first '7' for user permission mask.

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

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

Yep, 1.2.6 should be released any minute now.

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

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

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

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20140702/b378a9e9/attachment.sig>