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

[Martin Kletzander]

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

I just found out that the os.open() does create file with 755 (with
the default umask), but when I use (the preferred) open() it creates
it with 644.  Still can't explain that, though.

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



>_______________________________________________
>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/c4f2527f/attachment.sig>