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

[Martin Kletzander]

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.

>https://docs.python.org/2/library/os.html#os.open
>
>It's still a (potential) security issue even with a 'normal' umask
>though, since unless the directory the image is created in is
>restricted, any user could read it (and so any passwords or other
>secrets that might be in it), between the time it is (being) created
>and the time the person who created it gets around to noticing and
>fixing that.
>
>And having the executable bits set just seems wrong, unless I'm
>missing something that might be useful for (binfmt-misc?).
>If so that would seem like the exceptional case still ...
>

that's definitely true as well, I wonder why os.open() creates the
file with a+x permissions, but any C code that does the same creates
it with a-x.

>Is there a good reason for these to ever be more than 640 by default?
>
>  Cheers,
>  Ron
>
>
>> $ rm -f asdf.txt
>> $ python2 -c "import os; f = os.open('asdf.txt', os.O_WRONLY | os.O_CREAT); os.close(f)"
>> $ ls -al asdf.txt
>> -rwxr-xr-x 1 nert nert 0 Jun 30 14:36 asdf.txt
>> $ rm -f asdf.txt
>> $ python3 -c "import os; f = os.open('asdf.txt', os.O_WRONLY | os.O_CREAT); os.close(f)"
>> $ ls -al asdf.txt
>> -rwxr-xr-x 1 nert nert 0 Jun 30 14:37 asdf.txt
>>
>>
>> That would be a huge security issue if 0777 was the default.
>>
>> Martin
>>
>> >Disk image files don't need to be executable, and having them world
>> >readable isn't an ideal situation either.  Owner writable and group
>> >readable is probably more than sufficient when initially creating
>> >them.
>> >
>> >Signed-off-by: Ron Lee <ron at debian.org>
>> >---
>> >virtinst/diskbackend.py | 4 ++--
>> >virtinst/urlfetcher.py  | 2 +-
>> >2 files changed, 3 insertions(+), 3 deletions(-)
>> >
>> >diff --git a/virtinst/diskbackend.py b/virtinst/diskbackend.py
>> >index 5f72d00..2c74a11 100644
>> >--- a/virtinst/diskbackend.py
>> >+++ b/virtinst/diskbackend.py
>> >@@ -383,7 +383,7 @@ class StorageCreator(_StorageBase):
>> >            sparse = True
>> >            fd = None
>> >            try:
>> >-                fd = os.open(self._path, os.O_WRONLY | os.O_CREAT)
>> >+                fd = os.open(self._path, os.O_WRONLY | os.O_CREAT, 0640)
>> >                os.ftruncate(fd, size_bytes)
>> >            finally:
>> >                if fd:
>> >@@ -401,7 +401,7 @@ class StorageCreator(_StorageBase):
>> >        try:
>> >            try:
>> >                src_fd = os.open(self._clone_path, os.O_RDONLY)
>> >-                dst_fd = os.open(self._path, os.O_WRONLY | os.O_CREAT)
>> >+                dst_fd = os.open(self._path, os.O_WRONLY | os.O_CREAT, 0640)
>> >
>> >                i = 0
>> >                while 1:
>> >diff --git a/virtinst/urlfetcher.py b/virtinst/urlfetcher.py
>> >index 3f2744b..4e61814 100644
>> >--- a/virtinst/urlfetcher.py
>> >+++ b/virtinst/urlfetcher.py
>> >@@ -67,7 +67,7 @@ class _ImageFetcher(object):
>> >        prefix = "virtinst-" + prefix
>> >        if "VIRTINST_TEST_SUITE" in os.environ:
>> >            fn = os.path.join(".", prefix)
>> >-            fd = os.open(fn, os.O_RDWR | os.O_CREAT)
>> >+            fd = os.open(fn, os.O_RDWR | os.O_CREAT, 0640)
>> >        else:
>> >            (fd, fn) = tempfile.mkstemp(prefix=prefix,
>> >                                        dir=self.scratchdir)
>> >--
>> >2.0.0.rc2
>> >
>> >_______________________________________________
>> >virt-tools-list mailing list
>> >virt-tools-list at redhat.com
>> >https://www.redhat.com/mailman/listinfo/virt-tools-list
>
>
-------------- 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/20140701/017a2668/attachment.sig>