From 23e3c9b73b830ce04c94a1202f7cc8e684e3cd82 Mon Sep 17 00:00:00 2001
From: Michael Hanselmann <hansmi@google.com>
Date: Wed, 17 Oct 2012 16:10:07 +0200
Subject: [PATCH] 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>
---
lib/backend.py | 1 +
lib/bdev.py | 63 +++++++++++++++++++++++++++-
test/ganeti.bdev_unittest.py | 79 ++++++++++++++++++++++++++++++++++--
3 files changed, 137 insertions(+), 6 deletions(-)
diff --git a/lib/backend.py b/lib/backend.py
index 032547d22..6da70d609 100644
--- 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 8d41f53b6..d544675aa 100644
--- 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 e90071977..d58ee226f 100755
--- 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",
--
GitLab