Blob default permissions (#156)

* Fixed: A blob misfeature set blob permissions so that blobs and blob
  directories were only readable by the database process owner, rather
  than honoring user-controlled permissions (e.g. umask).

CHANGES.rst

@@ -5,7 +5,9 @@
 5.2.2 (unreleased)
 ==================
 
-- Nothing changed yet.
+- Fixed: A blob misfeature set blob permissions so that blobs and blob
+  directories were only readable by the database process owner, rather
+  than honoring user-controlled permissions (e.g. ``umask``).
 
 
 5.2.1 (2017-04-08)

src/ZODB/Connection.py

@@ -1199,7 +1199,7 @@ class TmpStore:
 
         targetpath = self._getBlobPath()
         if not os.path.exists(targetpath):
-            os.makedirs(targetpath, 0o700)
+            os.makedirs(targetpath)
 
         targetname = self._getCleanFilename(oid, serial)
         rename_or_copy_blob(blobfilename, targetname, chmod=False)

src/ZODB/FileStorage/FileStorage.py

@@ -1312,14 +1312,14 @@ class FileStorage(
 
         if self.pack_keep_old:
             # Helpers that move oid dir or revision file to the old dir.
-            os.mkdir(old, 0o777)
+            os.mkdir(old)
             link_or_copy(os.path.join(self.blob_dir, '.layout'),
                          os.path.join(old, '.layout'))
             def handle_file(path):
                 newpath = old+path[lblob_dir:]
                 dest = os.path.dirname(newpath)
                 if not os.path.exists(dest):
-                    os.makedirs(dest, 0o700)
+                    os.makedirs(dest)
                 os.rename(path, newpath)
             handle_dir = handle_file
         else:
@@ -1368,7 +1368,7 @@ class FileStorage(
                 file_path = os.path.join(path, file_name)
                 dest = os.path.dirname(old+file_path[lblob_dir:])
                 if not os.path.exists(dest):
-                    os.makedirs(dest, 0o700)
+                    os.makedirs(dest)
                 link_or_copy(file_path, old+file_path[lblob_dir:])
 
     def iterator(self, start=None, stop=None):

src/ZODB/blob.py

@@ -288,6 +288,7 @@ class Blob(persistent.Persistent):
             tempdir = self._p_jar.db()._storage.temporaryDirectory()
         else:
             tempdir = tempfile.gettempdir()
+
         filename = utils.mktemp(dir=tempdir, prefix="BUC")
         self._p_blob_uncommitted = filename
 
@@ -366,11 +367,11 @@ class FilesystemHelper:
 
     def create(self):
         if not os.path.exists(self.base_dir):
-            os.makedirs(self.base_dir, 0o700)
+            os.makedirs(self.base_dir)
             log("Blob directory '%s' does not exist. "
                 "Created new directory." % self.base_dir)
         if not os.path.exists(self.temp_dir):
-            os.makedirs(self.temp_dir, 0o700)
+            os.makedirs(self.temp_dir)
             log("Blob temporary directory '%s' does not exist. "
                 "Created new directory." % self.temp_dir)
 
@@ -388,13 +389,14 @@ class FilesystemHelper:
                     (self.layout_name, self.base_dir, layout))
 
     def isSecure(self, path):
-        """Ensure that (POSIX) path mode bits are 0700."""
-        return (os.stat(path).st_mode & 0o77) == 0
+        warnings.warn(
+            "isSecure is deprecated. Permissions are no longer set by ZODB",
+            DeprecationWarning, stacklevel=2)
 
     def checkSecure(self):
-        if not self.isSecure(self.base_dir):
-            log('Blob dir %s has insecure mode setting' % self.base_dir,
-                level=logging.WARNING)
+        warnings.warn(
+            "checkSecure is deprecated. Permissions are no longer set by ZODB",
+            DeprecationWarning, stacklevel=2)
 
     def getPathForOID(self, oid, create=False):
         """Given an OID, return the path on the filesystem where
@@ -414,7 +416,7 @@ class FilesystemHelper:
 
         if create and not os.path.exists(path):
             try:
-                os.makedirs(path, 0o700)
+                os.makedirs(path)
             except OSError:
                 # We might have lost a race.  If so, the directory
                 # must exist now
@@ -632,7 +634,6 @@ class BlobStorageMixin(object):
         # XXX Log warning if storage is ClientStorage
         self.fshelper = FilesystemHelper(blob_dir, layout)
         self.fshelper.create()
-        self.fshelper.checkSecure()
         self.dirty_oids = []
 
     def _blob_init_no_blobs(self):
@@ -908,8 +909,9 @@ def rename_or_copy_blob(f1, f2, chmod=True):
             with open(f2, 'wb') as file2:
                 utils.cp(file1, file2)
         remove_committed(f1)
+
     if chmod:
-        os.chmod(f2, stat.S_IREAD)
+        set_not_writable(f2)
 
 if sys.platform == 'win32':
     # On Windows, you can't remove read-only files, so make the
@@ -982,3 +984,17 @@ def copyTransactionsFromTo(source, destination):
 
         destination.tpc_vote(trans)
         destination.tpc_finish(trans)
+
+
+NO_WRITE = ~ (stat.S_IWUSR | stat.S_IWGRP | stat.S_IWOTH)
+READ_PERMS = stat.S_IRUSR | stat.S_IRGRP | stat.S_IROTH
+def set_not_writable(path):
+    perms = stat.S_IMODE(os.lstat(path).st_mode)
+
+    # Not writable:
+    perms &= NO_WRITE
+
+    # Read perms from folder:
+    perms |= stat.S_IMODE(os.lstat(os.path.dirname(path)).st_mode) & READ_PERMS
+
+    os.chmod(path, perms)

src/ZODB/tests/testblob.py

@@ -478,55 +478,31 @@ def packing_with_uncommitted_data_undoing():
     >>> database.close()
     """
 
-
-def secure_blob_directory():
+def test_blob_file_permissions():
     """
-    This is a test for secure creation and verification of secure settings of
-    blob directories.
-
-    >>> blob_storage = create_storage(blob_dir='blobs')
-
-    Two directories are created:
-
-    >>> os.path.isdir('blobs')
-    True
-    >>> tmp_dir = os.path.join('blobs', 'tmp')
-    >>> os.path.isdir(tmp_dir)
-    True
-
-    They are only accessible by the owner:
-
-    >>> oct(os.stat('blobs').st_mode)[-5:]
-    '40700'
-    >>> oct(os.stat(tmp_dir).st_mode)[-5:]
-    '40700'
+    >>> blob_storage = create_storage()
+    >>> conn = ZODB.connection(blob_storage)
+    >>> conn.root.x = ZODB.blob.Blob(b'test')
+    >>> conn.transaction_manager.commit()
 
-    These settings are recognized as secure:
+    Blobs have the readability of their parent directories:
 
-    >>> blob_storage.fshelper.isSecure('blobs')
-    True
-    >>> blob_storage.fshelper.isSecure(tmp_dir)
+    >>> import stat
+    >>> READABLE = stat.S_IRUSR | stat.S_IRGRP | stat.S_IROTH
+    >>> path = conn.root.x.committed()
+    >>> ((os.stat(path).st_mode & READABLE) ==
+    ...  (os.stat(os.path.dirname(path)).st_mode & READABLE))
     True
 
-    After making the permissions of tmp_dir more liberal, the directory is
-    recognized as insecure:
+    The committed file isn't writable:
 
-    >>> os.chmod(tmp_dir, 0o40711)
-    >>> blob_storage.fshelper.isSecure(tmp_dir)
-    False
-
-    Clean up:
-
-    >>> blob_storage.close()
+    >>> WRITABLE = stat.S_IWUSR | stat.S_IWGRP | stat.S_IWOTH
+    >>> os.stat(path).st_mode & WRITABLE
+    0
 
+    >>> conn.close()
     """
 
-# On windows, we can't create secure blob directories, at least not
-# with APIs in the standard library, so there's no point in testing
-# this.
-if sys.platform == 'win32':
-    del secure_blob_directory
-
 def loadblob_tmpstore():
     """
     This is a test for assuring that the TmpStore's loadBlob implementation