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

[Ron]

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.

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

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