Build storage: add additional checks for the source dir (#9890)

Extra checks for symlinks and making sure the
directory is within the docroot directory.

THIS DOESN'T PRESENT A SECURITY VULNERABILITY FOR RTD.ORG NOR RTD.COM,
since while fixing GHSA-368m-86q9-m99w
we added checks to make sure we didn't read files outside the docroot,
this limiting the attack surface, and we run one build at a time,
this means that only files from the current build are exposed in the docroot directory.

Author: stsewd

readthedocs/builds/storage.py

@@ -84,6 +84,7 @@ def copy_directory(self, source, destination):
             destination=destination,
         )
         source = Path(source)
+        self._check_suspicious_path(source)
         for filepath in source.iterdir():
             sub_destination = self.join(destination, filepath.name)
 
@@ -102,6 +103,21 @@ def copy_directory(self, source, destination):
                 with safe_open(filepath, "rb") as fd:
                     self.save(sub_destination, fd)
 
+    def _check_suspicious_path(self, path):
+        """Check that the given path isn't a symlink or outside the doc root."""
+        path = Path(path)
+        resolved_path = path.resolve()
+        if path.is_symlink():
+            msg = "Suspicious operation over a symbolic link."
+            log.error(msg, path=str(path), resolved_path=str(resolved_path))
+            raise SuspiciousFileOperation(msg)
+
+        docroot = Path(settings.DOCROOT).absolute()
+        if not path.is_relative_to(docroot):
+            msg = "Suspicious operation outside the docroot directory."
+            log.error(msg, path=str(path), resolved_path=str(resolved_path))
+            raise SuspiciousFileOperation(msg)
+
     def sync_directory(self, source, destination):
         """
         Sync a directory recursively to storage.
@@ -115,12 +131,15 @@ def sync_directory(self, source, destination):
         if destination in ('', '/'):
             raise SuspiciousFileOperation('Syncing all storage cannot be right')
 
+        source = Path(source)
+        self._check_suspicious_path(source)
+
         log.debug(
             'Syncing to media storage.',
-            source=source,
+            source=str(source),
             destination=destination,
         )
-        source = Path(source)
+
         copied_files = set()
         copied_dirs = set()
         for filepath in source.iterdir():

readthedocs/rtd_tests/tests/test_build_storage.py

@@ -1,7 +1,10 @@
 import os
 import shutil
 import tempfile
+from pathlib import Path
 
+import pytest
+from django.core.exceptions import SuspiciousFileOperation
 from django.test import TestCase, override_settings
 
 from readthedocs.builds.storage import BuildMediaFileSystemStorage
@@ -84,6 +87,42 @@ def test_sync_directory(self):
             self.storage.sync_directory(tmp_files_dir, storage_dir)
         self.assertFileTree(storage_dir, tree)
 
+    def test_sync_directory_source_symlink(self):
+        tmp_dir = Path(tempfile.mkdtemp())
+        tmp_symlink_dir = Path(tempfile.mkdtemp()) / "files"
+        tmp_symlink_dir.symlink_to(tmp_dir)
+
+        with override_settings(DOCROOT=tmp_dir):
+            with pytest.raises(SuspiciousFileOperation, match="symbolic link"):
+                self.storage.sync_directory(tmp_symlink_dir, "files")
+
+    def test_copy_directory_source_symlink(self):
+        tmp_dir = Path(tempfile.mkdtemp())
+        tmp_symlink_dir = Path(tempfile.mkdtemp()) / "files"
+        tmp_symlink_dir.symlink_to(tmp_dir)
+
+        with override_settings(DOCROOT=tmp_dir):
+            with pytest.raises(SuspiciousFileOperation, match="symbolic link"):
+                self.storage.copy_directory(tmp_symlink_dir, "files")
+
+    def test_sync_directory_source_outside_docroot(self):
+        tmp_dir = Path(tempfile.mkdtemp())
+        tmp_docroot = Path(tempfile.mkdtemp()) / "docroot"
+        tmp_docroot.mkdir()
+
+        with override_settings(DOCROOT=tmp_docroot):
+            with pytest.raises(SuspiciousFileOperation, match="outside the docroot"):
+                self.storage.sync_directory(tmp_dir, "files")
+
+    def test_copy_directory_source_outside_docroot(self):
+        tmp_dir = Path(tempfile.mkdtemp())
+        tmp_docroot = Path(tempfile.mkdtemp()) / "docroot"
+        tmp_docroot.mkdir()
+
+        with override_settings(DOCROOT=tmp_docroot):
+            with pytest.raises(SuspiciousFileOperation, match="outside the docroot"):
+                self.storage.copy_directory(tmp_dir, "files")
+
     def test_delete_directory(self):
         with override_settings(DOCROOT=files_dir):
             self.storage.copy_directory(files_dir, "files")