Symlink following: Arbitrary file access from builder server

This vulnerability allowed a malicious user to access any files that our application has read access to.
Exploiting this vulnerability required creating symlinks that pointed to files outside the user's project root.

This issue has been patched in our 9.0.1 release.

- [update with commit]

If you have any questions or comments about this advisory:
* Email us at [security@readthedocs.org](mailto:security@readthedocs.org) ([PGP](https://docs.readthedocs.io/page/security.html#pgp-key))

readthedocs/builds/storage.py

@@ -1,11 +1,13 @@
-import structlog
 from pathlib import Path
 
+import structlog
 from django.conf import settings
 from django.core.exceptions import SuspiciousFileOperation
 from django.core.files.storage import FileSystemStorage
 from storages.utils import get_available_overwrite_name, safe_join
 
+from readthedocs.core.utils.filesystem import safe_open
+
 log = structlog.get_logger(__name__)
 
 
@@ -84,11 +86,20 @@ def copy_directory(self, source, destination):
         source = Path(source)
         for filepath in source.iterdir():
             sub_destination = self.join(destination, filepath.name)
+
+            # Don't follow symlinks when uploading to storage.
+            if filepath.is_symlink():
+                log.info(
+                    "Skipping symlink upload.",
+                    path_resolved=str(filepath.resolve()),
+                )
+                continue
+
             if filepath.is_dir():
                 # Recursively copy the subdirectory
                 self.copy_directory(filepath, sub_destination)
             elif filepath.is_file():
-                with filepath.open('rb') as fd:
+                with safe_open(filepath, "rb") as fd:
                     self.save(sub_destination, fd)
 
     def sync_directory(self, source, destination):
@@ -114,12 +125,20 @@ def sync_directory(self, source, destination):
         copied_dirs = set()
         for filepath in source.iterdir():
             sub_destination = self.join(destination, filepath.name)
+            # Don't follow symlinks when uploading to storage.
+            if filepath.is_symlink():
+                log.info(
+                    "Skipping symlink upload.",
+                    path_resolved=str(filepath.resolve()),
+                )
+                continue
+
             if filepath.is_dir():
                 # Recursively sync the subdirectory
                 self.sync_directory(filepath, sub_destination)
                 copied_dirs.add(filepath.name)
             elif filepath.is_file():
-                with filepath.open('rb') as fd:
+                with safe_open(filepath, "rb") as fd:
                     self.save(sub_destination, fd)
                 copied_files.add(filepath.name)
 

readthedocs/config/config.py

@@ -11,6 +11,7 @@
 from django.conf import settings
 
 from readthedocs.config.utils import list_to_dict, to_dict
+from readthedocs.core.utils.filesystem import safe_open
 from readthedocs.projects.constants import GENERIC
 
 from .find import find_one
@@ -1380,7 +1381,11 @@ def load(path, env_config):
 
     if not filename:
         raise ConfigFileNotFound(path)
-    with open(filename, 'r') as configuration_file:
+
+    # Allow symlinks, but only the ones that resolve inside the base directory.
+    with safe_open(
+        filename, "r", allow_symlinks=True, base_path=path
+    ) as configuration_file:
         try:
             config = parse(configuration_file.read())
         except ParseError as error:

readthedocs/config/tests/test_config.py

@@ -6,6 +6,7 @@
 
 import pytest
 from django.conf import settings
+from django.test import override_settings
 from pytest import raises
 
 from readthedocs.config import (
@@ -80,7 +81,8 @@ def test_load_no_config_file(tmpdir, files):
     apply_fs(tmpdir, files)
     base = str(tmpdir)
     with raises(ConfigFileNotFound) as e:
-        load(base, {})
+        with override_settings(DOCROOT=tmpdir):
+            load(base, {})
     assert e.value.code == CONFIG_FILE_REQUIRED
 
 
@@ -92,13 +94,15 @@ def test_load_empty_config_file(tmpdir):
     )
     base = str(tmpdir)
     with raises(ConfigError):
-        load(base, {})
+        with override_settings(DOCROOT=tmpdir):
+            load(base, {})
 
 
 def test_minimal_config(tmpdir):
     apply_fs(tmpdir, yaml_config_dir)
     base = str(tmpdir)
-    build = load(base, {})
+    with override_settings(DOCROOT=tmpdir):
+        build = load(base, {})
     assert isinstance(build, BuildConfigV1)
 
 
@@ -111,7 +115,8 @@ def test_load_version1(tmpdir):
         },
     )
     base = str(tmpdir)
-    build = load(base, {})
+    with override_settings(DOCROOT=tmpdir):
+        build = load(base, {})
     assert isinstance(build, BuildConfigV1)
 
 
@@ -124,7 +129,8 @@ def test_load_version2(tmpdir):
         },
     )
     base = str(tmpdir)
-    build = load(base, {})
+    with override_settings(DOCROOT=tmpdir):
+        build = load(base, {})
     assert isinstance(build, BuildConfigV2)
 
 
@@ -138,7 +144,8 @@ def test_load_unknow_version(tmpdir):
     )
     base = str(tmpdir)
     with raises(ConfigError) as excinfo:
-        load(base, {})
+        with override_settings(DOCROOT=tmpdir):
+            load(base, {})
     assert excinfo.value.code == VERSION_INVALID
 
 
@@ -159,7 +166,8 @@ def test_load_raise_exception_invalid_syntax(tmpdir):
     )
     base = str(tmpdir)
     with raises(ConfigError) as excinfo:
-        load(base, {})
+        with override_settings(DOCROOT=tmpdir):
+            load(base, {})
     assert excinfo.value.code == CONFIG_SYNTAX_INVALID
 
 
@@ -176,13 +184,15 @@ def test_yaml_extension(tmpdir):
         },
     )
     base = str(tmpdir)
-    config = load(base, {})
+    with override_settings(DOCROOT=tmpdir):
+        config = load(base, {})
     assert isinstance(config, BuildConfigV1)
 
 
 def test_build_config_has_source_file(tmpdir):
     base = str(apply_fs(tmpdir, yaml_config_dir))
-    build = load(base, {})
+    with override_settings(DOCROOT=tmpdir):
+        build = load(base, {})
     assert build.source_file == os.path.join(base, 'readthedocs.yml')
 
 
@@ -196,7 +206,8 @@ def test_build_config_has_list_with_single_empty_value(tmpdir):
             ),
         },
     ))
-    build = load(base, {})
+    with override_settings(DOCROOT=tmpdir):
+        build = load(base, {})
     assert isinstance(build, BuildConfigV1)
     assert build.formats == []
 
@@ -682,7 +693,8 @@ def test_load_calls_validate(tmpdir):
     apply_fs(tmpdir, yaml_config_dir)
     base = str(tmpdir)
     with patch.object(BuildConfigV1, 'validate') as build_validate:
-        load(base, {})
+        with override_settings(DOCROOT=tmpdir):
+            load(base, {})
         assert build_validate.call_count == 1
 
 

readthedocs/core/tests/test_filesystem_utils.py

@@ -0,0 +1,181 @@
+from pathlib import Path
+from tempfile import mkdtemp
+
+import pytest
+from django.core.exceptions import SuspiciousFileOperation
+from django.test import TestCase, override_settings
+
+from readthedocs.core.utils.filesystem import safe_copytree, safe_open, safe_rmtree
+from readthedocs.doc_builder.exceptions import (
+    FileIsNotRegularFile,
+    SymlinkOutsideBasePath,
+    UnsupportedSymlinkFileError,
+)
+
+
+class TestFileSystemUtils(TestCase):
+    def assert_files_equal(self, directory, files):
+        self.assertEqual(
+            {str(p.relative_to(directory)) for p in directory.iterdir()}, files
+        )
+
+    def test_copytree(self):
+        from_directory = Path(mkdtemp())
+        docroot_path = from_directory.parent
+        to_directory = Path(mkdtemp()) / "target"
+
+        (from_directory / "test.txt").touch()
+
+        self.assertFalse(to_directory.exists())
+
+        with override_settings(DOCROOT=docroot_path):
+            safe_copytree(from_directory, to_directory)
+
+        self.assert_files_equal(to_directory, {"test.txt"})
+
+    def test_copytree_outside_docroot(self):
+        from_directory = Path(mkdtemp())
+        (from_directory / "test.txt").touch()
+        to_directory = Path(mkdtemp()) / "target"
+        docroot_path = Path(mkdtemp())
+
+        with pytest.raises(SuspiciousFileOperation):
+            with override_settings(DOCROOT=docroot_path):
+                safe_copytree(from_directory, to_directory)
+
+    def test_copytree_with_symlinks(self):
+        from_directory = Path(mkdtemp())
+        docroot_path = from_directory.parent
+        to_directory = Path(mkdtemp()) / "target"
+
+        file_a = from_directory / "test.txt"
+        file_a.touch()
+
+        symlink_a = from_directory / "symlink.txt"
+        symlink_a.symlink_to(file_a)
+        symlink_b = from_directory / "symlink-dir"
+        symlink_b.symlink_to(to_directory.parent)
+
+        self.assertFalse(to_directory.exists())
+
+        with override_settings(DOCROOT=docroot_path):
+            safe_copytree(from_directory, to_directory)
+
+        # Symlinks are copied as symlinks, not as files.
+        self.assert_files_equal(
+            to_directory, {"test.txt", "symlink.txt", "symlink-dir"}
+        )
+        self.assertTrue((to_directory / "symlink.txt").is_symlink())
+        self.assertTrue((to_directory / "symlink-dir").is_symlink())
+
+    def test_copytree_from_dir_as_symlink(self):
+        root_directory = Path(mkdtemp())
+        docroot_path = root_directory
+        from_directory = root_directory / "a"
+        from_directory.mkdir()
+        (from_directory / "test.txt").touch()
+
+        to_directory = root_directory / "b"
+
+        from_directory_symlink = root_directory / "symlink-a"
+        from_directory_symlink.symlink_to(from_directory)
+
+        self.assertFalse(to_directory.exists())
+
+        with override_settings(DOCROOT=docroot_path):
+            self.assertFalse(safe_copytree(from_directory_symlink, to_directory))
+
+        self.assertFalse(to_directory.exists())
+
+    def test_open(self):
+        root_directory = Path(mkdtemp())
+        docroot_path = root_directory
+        file_a = root_directory / "test.txt"
+        file_a.touch()
+
+        with override_settings(DOCROOT=docroot_path):
+            context_manager = safe_open(file_a, allow_symlinks=False)
+            self.assertIsNotNone(context_manager)
+
+        with override_settings(DOCROOT=docroot_path):
+            context_manager = safe_open(
+                file_a, allow_symlinks=True, base_path=root_directory
+            )
+            self.assertIsNotNone(context_manager)
+
+    def test_open_outside_docroot(self):
+        root_directory = Path(mkdtemp())
+        docroot_path = Path(mkdtemp())
+        file_a = root_directory / "test.txt"
+        file_a.touch()
+
+        with pytest.raises(SuspiciousFileOperation):
+            with override_settings(DOCROOT=docroot_path):
+                safe_open(file_a)
+
+    def test_open_with_symlinks(self):
+        root_directory = Path(mkdtemp())
+        docroot_path = root_directory
+        file_a = root_directory / "test.txt"
+        file_a.touch()
+
+        symlink_a = root_directory / "symlink.txt"
+        symlink_a.symlink_to(file_a)
+
+        # Symlinks aren't allowed.
+        with pytest.raises(UnsupportedSymlinkFileError):
+            with override_settings(DOCROOT=docroot_path):
+                safe_open(symlink_a, allow_symlinks=False)
+
+        # Symlinks are allowed if they are under the root_directory.
+        with override_settings(DOCROOT=docroot_path):
+            context_manager = safe_open(
+                symlink_a, allow_symlinks=True, base_path=root_directory
+            )
+            self.assertIsNotNone(context_manager)
+
+        # Symlinks aren't allowed if they aren't under the root_directory.
+        with pytest.raises(SymlinkOutsideBasePath):
+            with override_settings(DOCROOT=docroot_path):
+                new_root_directory = root_directory / "dir"
+                new_root_directory.mkdir()
+                safe_open(symlink_a, allow_symlinks=True, base_path=new_root_directory)
+
+    def test_rmtree(self):
+        root_directory = Path(mkdtemp())
+        docroot_path = root_directory
+        (root_directory / "test.txt").touch()
+
+        self.assertTrue(root_directory.exists())
+
+        with override_settings(DOCROOT=docroot_path):
+            safe_rmtree(root_directory)
+            self.assertFalse(root_directory.exists())
+
+    def test_rmtree_outside_docroot(self):
+        root_directory = Path(mkdtemp())
+        docroot_path = Path(mkdtemp())
+        (root_directory / "test.txt").touch()
+
+        self.assertTrue(root_directory.exists())
+
+        with pytest.raises(SuspiciousFileOperation):
+            with override_settings(DOCROOT=docroot_path):
+                safe_rmtree(root_directory)
+
+    def test_rmtree_with_symlinks(self):
+        root_directory = Path(mkdtemp())
+        docroot_path = root_directory
+
+        dir_a = root_directory / "test"
+        dir_a.mkdir()
+        (dir_a / "test.txt").touch()
+
+        symlink_a = root_directory / "symlink"
+        symlink_a.symlink_to(dir_a)
+
+        # Directories that point to a symlink aren't deleted.
+        self.assertTrue(symlink_a.exists())
+        with override_settings(DOCROOT=docroot_path):
+            safe_rmtree(symlink_a)
+            self.assertTrue(symlink_a.exists())