bdev: Add verification for file storage paths

An earlier version of this patch series verified all paths in cmdlib in
the master daemon. With this change all that verification code is moved
to bdev to run inside the node daemon. The checks are much stricter
now--it is no longer possible to use forbidden paths (e.g. /bin) to
manipulate file storage devices (once these checks are being used).

Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

diff --git a/lib/backend.py b/lib/backend.py
index 032547d..6da70d6 100644 (file)
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -800,6 +800,7 @@ def VerifyNode(what, cluster_name):
     result[constants.NV_BRIDGES] = [bridge
                                     for bridge in what[constants.NV_BRIDGES]
                                     if not utils.BridgeExists(bridge)]
+
   return result
 
 

diff --git a/lib/bdev.py b/lib/bdev.py
index 8d41f53..d544675 100644 (file)
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -29,6 +29,7 @@ import stat
 import pyparsing as pyp
 import os
 import logging
+import itertools
 
 from ganeti import utils
 from ganeti import errors
@@ -100,6 +101,58 @@ def _CanReadDevice(path):
     return False
 
 
+def _GetForbiddenFileStoragePaths():
+  """Builds a list of path prefixes which shouldn't be used for file storage.
+
+  @rtype: frozenset
+
+  """
+  paths = set([
+    "/boot",
+    "/dev",
+    "/etc",
+    "/home",
+    "/proc",
+    "/root",
+    "/sys",
+    ])
+
+  for prefix in ["", "/usr", "/usr/local"]:
+    paths.update(map(lambda s: "%s/%s" % (prefix, s),
+                     ["bin", "lib", "lib32", "lib64", "sbin"]))
+
+  return frozenset(map(os.path.normpath, paths))
+
+
+def _ComputeWrongFileStoragePaths(paths,
+                                  _forbidden=_GetForbiddenFileStoragePaths()):
+  """Cross-checks a list of paths for prefixes considered bad.
+
+  Some paths, e.g. "/bin", should not be used for file storage.
+
+  @type paths: list
+  @param paths: List of paths to be checked
+  @rtype: list
+  @return: Sorted list of paths for which the user should be warned
+
+  """
+  def _Check(path):
+    return (not os.path.isabs(path) or
+            path in _forbidden or
+            filter(lambda p: utils.IsBelowDir(p, path), _forbidden))
+
+  return utils.NiceSort(filter(_Check, map(os.path.normpath, paths)))
+
+
+def ComputeWrongFileStoragePaths(_filename=pathutils.FILE_STORAGE_PATHS_FILE):
+  """Returns a list of file storage paths whose prefix is considered bad.
+
+  See L{_ComputeWrongFileStoragePaths}.
+
+  """
+  return _ComputeWrongFileStoragePaths(_LoadAllowedFileStoragePaths(_filename))
+
+
 def _CheckFileStoragePath(path, allowed):
   """Checks if a path is in a list of allowed paths for file storage.
 
@@ -126,7 +179,7 @@ def _CheckFileStoragePath(path, allowed):
                                       " storage" % path)
 
 
-def LoadAllowedFileStoragePaths(filename):
+def _LoadAllowedFileStoragePaths(filename):
   """Loads file containing allowed file storage paths.
 
   @rtype: list
@@ -149,7 +202,13 @@ def CheckFileStoragePath(path, _filename=pathutils.FILE_STORAGE_PATHS_FILE):
   @raise errors.FileStoragePathError: If the path is not allowed
 
   """
-  _CheckFileStoragePath(path, LoadAllowedFileStoragePaths(_filename))
+  allowed = _LoadAllowedFileStoragePaths(_filename)
+
+  if _ComputeWrongFileStoragePaths([path]):
+    raise errors.FileStoragePathError("Path '%s' uses a forbidden prefix" %
+                                      path)
+
+  _CheckFileStoragePath(path, allowed)
 
 
 class BlockDev(object):

diff --git a/test/ganeti.bdev_unittest.py b/test/ganeti.bdev_unittest.py
index e900719..d58ee22 100755 (executable)
--- a/test/ganeti.bdev_unittest.py
+++ b/test/ganeti.bdev_unittest.py
@@ -373,7 +373,48 @@ class TestRADOSBlockDevice(testutils.GanetiTestCase):
                       output_extra_matches, volume_name)
 
 
-class TestCheckFileStoragePath(unittest.TestCase):
+class TestComputeWrongFileStoragePathsInternal(unittest.TestCase):
+  def testPaths(self):
+    paths = bdev._GetForbiddenFileStoragePaths()
+
+    for path in ["/bin", "/usr/local/sbin", "/lib64", "/etc", "/sys"]:
+      self.assertTrue(path in paths)
+
+    self.assertEqual(set(map(os.path.normpath, paths)), paths)
+
+  def test(self):
+    vfsp = bdev._ComputeWrongFileStoragePaths
+    self.assertEqual(vfsp([]), [])
+    self.assertEqual(vfsp(["/tmp"]), [])
+    self.assertEqual(vfsp(["/bin/ls"]), ["/bin/ls"])
+    self.assertEqual(vfsp(["/bin"]), ["/bin"])
+    self.assertEqual(vfsp(["/usr/sbin/vim", "/srv/file-storage"]),
+                     ["/usr/sbin/vim"])
+
+
+class TestComputeWrongFileStoragePaths(testutils.GanetiTestCase):
+  def test(self):
+    tmpfile = self._CreateTempFile()
+
+    utils.WriteFile(tmpfile, data="""
+      /tmp
+      x/y///z/relative
+      # This is a test file
+      /srv/storage
+      /bin
+      /usr/local/lib32/
+      relative/path
+      """)
+
+    self.assertEqual(bdev.ComputeWrongFileStoragePaths(_filename=tmpfile), [
+      "/bin",
+      "/usr/local/lib32",
+      "relative/path",
+      "x/y/z/relative",
+      ])
+
+
+class TestCheckFileStoragePathInternal(unittest.TestCase):
   def testNonAbsolute(self):
     for i in ["", "tmp", "foo/bar/baz"]:
       self.assertRaises(errors.FileStoragePathError,
@@ -395,14 +436,44 @@ class TestCheckFileStoragePath(unittest.TestCase):
     bdev._CheckFileStoragePath("/tmp/foo/a/x", ["/tmp/foo"])
 
 
+class TestCheckFileStoragePath(testutils.GanetiTestCase):
+  def testNonExistantFile(self):
+    filename = "/tmp/this/file/does/not/exist"
+    assert not os.path.exists(filename)
+    self.assertRaises(errors.FileStoragePathError,
+                      bdev.CheckFileStoragePath, "/bin/", _filename=filename)
+    self.assertRaises(errors.FileStoragePathError,
+                      bdev.CheckFileStoragePath, "/srv/file-storage",
+                      _filename=filename)
+
+  def testAllowedPath(self):
+    tmpfile = self._CreateTempFile()
+
+    utils.WriteFile(tmpfile, data="""
+      /srv/storage
+      """)
+
+    bdev.CheckFileStoragePath("/srv/storage/inst1", _filename=tmpfile)
+
+    # No additional path component
+    self.assertRaises(errors.FileStoragePathError,
+                      bdev.CheckFileStoragePath, "/srv/storage",
+                      _filename=tmpfile)
+
+    # Forbidden path
+    self.assertRaises(errors.FileStoragePathError,
+                      bdev.CheckFileStoragePath, "/usr/lib64/xyz",
+                      _filename=tmpfile)
+
+
 class TestLoadAllowedFileStoragePaths(testutils.GanetiTestCase):
   def testDevNull(self):
-    self.assertEqual(bdev.LoadAllowedFileStoragePaths("/dev/null"), [])
+    self.assertEqual(bdev._LoadAllowedFileStoragePaths("/dev/null"), [])
 
   def testNonExistantFile(self):
     filename = "/tmp/this/file/does/not/exist"
     assert not os.path.exists(filename)
-    self.assertEqual(bdev.LoadAllowedFileStoragePaths(filename), [])
+    self.assertEqual(bdev._LoadAllowedFileStoragePaths(filename), [])
 
   def test(self):
     tmpfile = self._CreateTempFile()
@@ -414,7 +485,7 @@ class TestLoadAllowedFileStoragePaths(testutils.GanetiTestCase):
       relative/path
       """)
 
-    self.assertEqual(bdev.LoadAllowedFileStoragePaths(tmpfile), [
+    self.assertEqual(bdev._LoadAllowedFileStoragePaths(tmpfile), [
       "/tmp",
       "/srv/storage",
       "relative/path",