Change in vdsm[master]: mkimage: create files with explicit permissions

Francesco Romani has uploaded a new change for review.

Change subject: mkimage: create files with explicit permissions
......................................................................

mkimage: create files with explicit permissions

Files in the ISO image, most notably `user_data` and `meta_data.json`
files on the config-drive have world readable permissions.
They contain sensitive data, so it is unsafe to do so.

This patch address the issue by adding the capability to set
per-file permissions at creation time in
mkimage._decodeFilesIntoDir
current default is 0o640, so we just avoid to create any world-readable
file at all.

Another approach to obtain exactly the same result could be to
leverate the -file-mode option of the `genisoimage` program.
But doing into the python code has the following advantages:

* creates the correct permissions right from the start, effectively
  eliding the vulnerability window; if we let genisoimage fix things,
  there is a (very short, indeed) time window on which the newly created
  files are still world-readable.

* this paves the road to fine-grained, per-file permissions
  with negligibile extra cost (in short, this is more forward-compatible).

Bug-Url: https://bugzilla.redhat.com/1034247

Signed-off-by: Francesco Romani <fromani(a)redhat.com&gt;
Change-Id: Id1a3baca2ec1bf2dd0762641633a749573c40147
---
M tests/mkimageTests.py
M vdsm/mkimage.py
2 files changed, 29 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/56/21956/1

diff --git a/tests/mkimageTests.py b/tests/mkimageTests.py
index 28b35bb..ed6df6a 100644
--- a/tests/mkimageTests.py
+++ b/tests/mkimageTests.py
@@ -28,6 +28,7 @@
 
 from base64 import b64encode
 import os
+import stat
 from shutil import rmtree
 from tempfile import mkdtemp
 
@@ -88,6 +89,17 @@
         #pylint: disable=W0212
         mkimage._P_PAYLOAD_IMAGES = self.orig_mkimage["_P_PAYLOAD_IMAGES"]
 
+    def _check_permissions(self, filepath, permsMask):
+        """
+        Ensure the file at `filepath' has the permissions coherent
+        with the given mask.
+        The mask may specifiy the required presence, or absence, of a
+        permission bit.
+        """
+        data = os.stat(filepath)
+        for perm, expected in permsMask:
+            self.assertEqual(bool(data.st_mode & perm), expected)
+
     def _check_content(self):
         """
         Ensure that the workdir contains what we want
@@ -101,7 +113,12 @@
                 self.assertTrue(filename in out_dir)
             else:
                 self.assertTrue(os.path.basename(filename) in out_subdir)
-            with open(os.path.join(self.workdir, filename), "r") as fd:
+            filepath = os.path.join(self.workdir, filename)
+            self._check_permissions(filepath,
+                                    ((stat.S_IROTH, False),
+                                     (stat.S_IWOTH, False),
+                                     (stat.S_IXOHT, False))
+            with open(filepath, "r") as fd:
                 content = fd.read()
                 self.assertEqual(content, self.expected_results[filename])
 
diff --git a/vdsm/mkimage.py b/vdsm/mkimage.py
index a4716c7..ca8585e 100644
--- a/vdsm/mkimage.py
+++ b/vdsm/mkimage.py
@@ -35,6 +35,16 @@
 _P_PAYLOAD_IMAGES = os.path.join(P_VDSM_RUN, 'payload')
 
 
+def _openFile(filename, mode, perms=0o644):
+    '''
+    opens a filename allowing to specify the unix permissions
+    right from the start, to avoid world-readable files
+    with sensitive informations.
+    '''
+    fd = os.open(filename, os.O_CREAT|os.O_TRUNC|os.O_RDWR, perms)
+    return os.fdopen(fd, mode)
+
+
 def _decodeFilesIntoDir(files, parentdir):
     '''
     create temp files from files list
@@ -55,7 +65,7 @@
             except OSError as e:
                 if e.errno != os.errno.EEXIST:
                     raise
-        with file(filename, 'w') as f:
+        with _openFile(filename, 'w', perms=0o640) as f:
             f.write(base64.b64decode(content))
 
 


-- 
To view, visit http://gerrit.ovirt.org/21956
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id1a3baca2ec1bf2dd0762641633a749573c40147
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com&gt;

Francesco Romani has abandoned this change.

Change subject: mkimage: create files with explicit permissions
......................................................................


Abandoned

mistakenly pushed too early

-- 
To view, visit http://gerrit.ovirt.org/21956
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: Id1a3baca2ec1bf2dd0762641633a749573c40147
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: mkimage: create files with explicit permissions
......................................................................


Patch Set 1: Code-Review-1 Verified-1

Build Failed 

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5873/ : UNSTABLE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5077/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5965/ : FAILURE

-- 
To view, visit http://gerrit.ovirt.org/21956
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id1a3baca2ec1bf2dd0762641633a749573c40147
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No