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@xxxxxxxxxx> >--- >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@xxxxxxxxxx >https://www.redhat.com/mailman/listinfo/virt-tools-list
Attachment:
signature.asc
Description: Digital signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
