fix: FileProvider.validate() leaked files outside root

Fixes two issues and adds two improvements:

* FileProvider would read files outside the root if the root were
  a prefix of the file path string-wise. Consider a root path
  `/tmp/sample/root` and a relative path `../root.txt`, resulting in
  reading a file `/tmp/sample/root.txt`.

* The "inside root" check was performed after checking if file existed.
  This would expose information about files outside the root through the
  exception. While an exploit would be difficult (if possible at all),
  it is better to fix this weakness anyway.

* The exception type is changed from the most generic `Exception` to
  `ValueError` to make it easier to distinguish it from
  `ContentException` that is raised in other cases.

* The `"/"` literal in path operations is changed to `os.sep` to improve
  portability.

Signed-off-by: Jan Holeček <71874510+jholecek-rh@users.noreply.github.com>

Author: jholecek-rh

insights/core/spec_factory.py

@@ -214,7 +214,22 @@ def __init__(self, relative_path, root="/", save_as=None, ds=None, ctx=None, cle
 
         self.validate()
 
+    def _is_inside_root(self):
+        """Checks that `self.relative_path` does not point outside `self.root`."""
+        resolved = os.path.realpath(self.path)
+
+        # pathlib.Path.is_relative_to() has been added only in Python 3.9
+        resolved_root = os.path.realpath(self.root)
+        if not resolved_root.endswith(os.sep):
+            resolved_root += os.sep
+
+        return resolved.startswith(resolved_root)
+
     def validate(self):
+        if not self._is_inside_root():
+            msg = "Relative path points outside the root: %s"
+            raise ValueError(msg % (self.path))
+
         # 1. No Such File
         if not os.path.exists(self.path):
             raise ContentException("%s does not exist." % self.path)
@@ -224,15 +239,10 @@ def validate(self):
             if self._filterable and not self._filters:
                 raise NoFilterException("Skipping %s due to no filters." % dr.get_name(self.ds))
             # 2.2 Customer Prohibits Collection
-            if not blacklist.allow_file("/" + self.relative_path):
-                log.warning("WARNING: Skipping file %s", "/" + self.relative_path)
+            if not blacklist.allow_file(os.sep + self.relative_path):
+                log.warning("WARNING: Skipping file %s", os.sep + self.relative_path)
                 raise BlacklistedSpec()
 
-        resolved = os.path.realpath(self.path)
-        if not resolved.startswith(os.path.realpath(self.root)):
-            msg = "Relative path points outside the root: %s -> %s."
-            raise Exception(msg % (self.path, resolved))
-
         if not os.access(self.path, os.R_OK):
             raise ContentException("Cannot access %s" % self.path)
 

insights/tests/core/spec_factory/test_file_provider.py

@@ -32,7 +32,8 @@ def create_file(relpath, content=""):
     create_file(SAMPLE_FILE)
     create_directory("sample/root")
     create_file("sample/root/access_denied.txt")
-    os.chmod(os.path.join(root, "sample/root/access_denied.txt"), 0)
+    os.chmod(os.path.join(root, "sample/root/access_denied.txt"), 0o000)
+    create_file("sample/root.txt")
     return root
 
 
@@ -48,7 +49,7 @@ def test_repr(sample_directory):
 
 
 @pytest.mark.skipif(
-    # GitHub workflows run Python 2.7 and 3.6 tests in containers, because these old Python
+    # GitHub workflows run Python 2.7 and 3.6 tests in containers because these old Python
     # versions are not available in GitHub-hosted runners anymore. The tests are executed as root.
     os.getuid() == 0,
     reason="Test must not be run as root. Root ignores file mode bits."
@@ -59,7 +60,11 @@ def test_repr(sample_directory):
         ("access_denied.txt", ContentException, "Cannot access"),
         ("no_such_file.txt", ContentException, "does not exist"),
         # file outside root that exists
-        ("../../sample_file.txt", Exception, "Relative path points outside the root"),
+        ("../../sample_file.txt", ValueError, "Relative path points outside the root"),
+        # file outside root that does not exist
+        ("../no_such_file.txt", ValueError, "Relative path points outside the root"),
+        # file outside root where root path is a prefix of the file path string-wise
+        ("../root.txt", ValueError, "Relative path points outside the root"),
     ]
 )
 def test_validate(sample_directory, relpath, exception_type, match):