[5.0.x] Fixed CVE-2024-39330 -- Added extra file name validation in Storage's save method.

nessita · nessita · commit 9f4f63e9ebb7 · 2024-07-09T10:03:32.000-03:00
Thanks to Josh Schneier for the report, and to Carlton Gibson and Sarah
Boyce for the reviews.
diff --git a/django/core/files/storage/base.py b/django/core/files/storage/base.py
@@ -34,7 +34,18 @@ def save(self, name, content, max_length=None):
         if not hasattr(content, "chunks"):
             content = File(content, name)
 
+        # Ensure that the name is valid, before and after having the storage
+        # system potentially modifying the name. This duplicates the check made
+        # inside `get_available_name` but it's necessary for those cases where
+        # `get_available_name` is overriden and validation is lost.
+        validate_file_name(name, allow_relative_path=True)
+
+        # Potentially find a different name depending on storage constraints.
         name = self.get_available_name(name, max_length=max_length)
+        # Validate the (potentially) new name.
+        validate_file_name(name, allow_relative_path=True)
+
+        # The save operation should return the actual name of the file saved.
         name = self._save(name, content)
         # Ensure that the name returned from the storage system is still valid.
         validate_file_name(name, allow_relative_path=True)
diff --git a/django/core/files/utils.py b/django/core/files/utils.py
@@ -10,10 +10,9 @@ def validate_file_name(name, allow_relative_path=False):
         raise SuspiciousFileOperation("Could not derive file name from '%s'" % name)
 
     if allow_relative_path:
-        # Use PurePosixPath() because this branch is checked only in
-        # FileField.generate_filename() where all file paths are expected to be
-        # Unix style (with forward slashes).
-        path = pathlib.PurePosixPath(name)
+        # Ensure that name can be treated as a pure posix path, i.e. Unix
+        # style (with forward slashes).
+        path = pathlib.PurePosixPath(str(name).replace("\\", "/"))
         if path.is_absolute() or ".." in path.parts:
             raise SuspiciousFileOperation(
                 "Detected path traversal attempt in '%s'" % name
diff --git a/docs/releases/4.2.14.txt b/docs/releases/4.2.14.txt
@@ -20,3 +20,15 @@ CVE-2024-39329: Username enumeration through timing difference for users with un
 The :meth:`~django.contrib.auth.backends.ModelBackend.authenticate()` method
 allowed remote attackers to enumerate users via a timing attack involving login
 requests for users with unusable passwords.
+
+CVE-2024-39330: Potential directory-traversal via ``Storage.save()``
+====================================================================
+
+Derived classes of the :class:`~django.core.files.storage.Storage` base class
+which override :meth:`generate_filename()
+<django.core.files.storage.Storage.generate_filename()>` without replicating
+the file path validations existing in the parent class, allowed for potential
+directory-traversal via certain inputs when calling :meth:`save()
+<django.core.files.storage.Storage.save()>`.
+
+Built-in ``Storage`` sub-classes were not affected by this vulnerability.
diff --git a/docs/releases/5.0.7.txt b/docs/releases/5.0.7.txt
@@ -21,6 +21,18 @@ The :meth:`~django.contrib.auth.backends.ModelBackend.authenticate()` method
 allowed remote attackers to enumerate users via a timing attack involving login
 requests for users with unusable passwords.
 
+CVE-2024-39330: Potential directory-traversal via ``Storage.save()``
+====================================================================
+
+Derived classes of the :class:`~django.core.files.storage.Storage` base class
+which override :meth:`generate_filename()
+<django.core.files.storage.Storage.generate_filename()>` without replicating
+the file path validations existing in the parent class, allowed for potential
+directory-traversal via certain inputs when calling :meth:`save()
+<django.core.files.storage.Storage.save()>`.
+
+Built-in ``Storage`` sub-classes were not affected by this vulnerability.
+
 Bugfixes
 ========
 
diff --git a/tests/file_storage/test_base.py b/tests/file_storage/test_base.py
@@ -0,0 +1,72 @@
+import os
+from unittest import mock
+
+from django.core.exceptions import SuspiciousFileOperation
+from django.core.files.storage import Storage
+from django.test import SimpleTestCase
+
+
+class CustomStorage(Storage):
+    """Simple Storage subclass implementing the bare minimum for testing."""
+
+    def exists(self, name):
+        return False
+
+    def _save(self, name):
+        return name
+
+
+class StorageValidateFileNameTests(SimpleTestCase):
+
+    invalid_file_names = [
+        os.path.join("path", "to", os.pardir, "test.file"),
+        os.path.join(os.path.sep, "path", "to", "test.file"),
+    ]
+    error_msg = "Detected path traversal attempt in '%s'"
+
+    def test_validate_before_get_available_name(self):
+        s = CustomStorage()
+        # The initial name passed to `save` is not valid nor safe, fail early.
+        for name in self.invalid_file_names:
+            with (
+                self.subTest(name=name),
+                mock.patch.object(s, "get_available_name") as mock_get_available_name,
+                mock.patch.object(s, "_save") as mock_internal_save,
+            ):
+                with self.assertRaisesMessage(
+                    SuspiciousFileOperation, self.error_msg % name
+                ):
+                    s.save(name, content="irrelevant")
+                self.assertEqual(mock_get_available_name.mock_calls, [])
+                self.assertEqual(mock_internal_save.mock_calls, [])
+
+    def test_validate_after_get_available_name(self):
+        s = CustomStorage()
+        # The initial name passed to `save` is valid and safe, but the returned
+        # name from `get_available_name` is not.
+        for name in self.invalid_file_names:
+            with (
+                self.subTest(name=name),
+                mock.patch.object(s, "get_available_name", return_value=name),
+                mock.patch.object(s, "_save") as mock_internal_save,
+            ):
+                with self.assertRaisesMessage(
+                    SuspiciousFileOperation, self.error_msg % name
+                ):
+                    s.save("valid-file-name.txt", content="irrelevant")
+                self.assertEqual(mock_internal_save.mock_calls, [])
+
+    def test_validate_after_internal_save(self):
+        s = CustomStorage()
+        # The initial name passed to `save` is valid and safe, but the result
+        # from `_save` is not (this is achieved by monkeypatching _save).
+        for name in self.invalid_file_names:
+            with (
+                self.subTest(name=name),
+                mock.patch.object(s, "_save", return_value=name),
+            ):
+
+                with self.assertRaisesMessage(
+                    SuspiciousFileOperation, self.error_msg % name
+                ):
+                    s.save("valid-file-name.txt", content="irrelevant")
diff --git a/tests/file_storage/tests.py b/tests/file_storage/tests.py
@@ -342,22 +342,17 @@ def test_file_save_with_path(self):
 
         self.storage.delete("path/to/test.file")
 
-    def test_file_save_abs_path(self):
-        test_name = "path/to/test.file"
-        f = ContentFile("file saved with path")
-        f_name = self.storage.save(os.path.join(self.temp_dir, test_name), f)
-        self.assertEqual(f_name, test_name)
-
     @unittest.skipUnless(
         symlinks_supported(), "Must be able to symlink to run this test."
     )
     def test_file_save_broken_symlink(self):
         """A new path is created on save when a broken symlink is supplied."""
         nonexistent_file_path = os.path.join(self.temp_dir, "nonexistent.txt")
-        broken_symlink_path = os.path.join(self.temp_dir, "symlink.txt")
+        broken_symlink_file_name = "symlink.txt"
+        broken_symlink_path = os.path.join(self.temp_dir, broken_symlink_file_name)
         os.symlink(nonexistent_file_path, broken_symlink_path)
         f = ContentFile("some content")
-        f_name = self.storage.save(broken_symlink_path, f)
+        f_name = self.storage.save(broken_symlink_file_name, f)
         self.assertIs(os.path.exists(os.path.join(self.temp_dir, f_name)), True)
 
     def test_save_doesnt_close(self):
diff --git a/tests/file_uploads/tests.py b/tests/file_uploads/tests.py
@@ -828,7 +828,7 @@ def test_not_a_directory(self):
         default_storage.delete(UPLOAD_TO)
         # Create a file with the upload directory name
         with SimpleUploadedFile(UPLOAD_TO, b"x") as file:
-            default_storage.save(UPLOAD_TO, file)
+            default_storage.save(UPLOAD_FOLDER, file)
         self.addCleanup(default_storage.delete, UPLOAD_TO)
         msg = "%s exists and is not a directory." % UPLOAD_TO
         with self.assertRaisesMessage(FileExistsError, msg):
