From fe4a0bbe2088d0c2b331216dad21ccd0bb3ee80d Mon Sep 17 00:00:00 2001 From: Natalia <124304+nessita@users.noreply.github.com> Date: Wed, 20 Mar 2024 13:55:21 -0300 Subject: [PATCH] Fixed CVE-2024-39330 -- Added extra file name validation in Storage's save method. Thanks to Josh Schneier for the report, and to Carlton Gibson and Sarah Boyce for the reviews. --- django/core/files/storage/base.py | 11 +++++ django/core/files/utils.py | 7 ++- docs/releases/4.2.14.txt | 12 ++++++ docs/releases/5.0.7.txt | 12 ++++++ tests/file_storage/test_base.py | 72 +++++++++++++++++++++++++++++++ tests/file_storage/tests.py | 11 ++--- tests/file_uploads/tests.py | 2 +- 7 files changed, 114 insertions(+), 13 deletions(-) create mode 100644 tests/file_storage/test_base.py diff --git a/django/core/files/storage/base.py b/django/core/files/storage/base.py index 6ce4ab2535..55285bc23a 100644 --- a/django/core/files/storage/base.py +++ b/django/core/files/storage/base.py @@ -34,7 +34,18 @@ class Storage: 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 index 611f932f6e..c730ca17e8 100644 --- 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 index 556cff4437..dc20cd9f28 100644 --- 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() +` without replicating +the file path validations existing in the parent class, allowed for potential +directory-traversal via certain inputs when calling :meth:`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 index 53efe3766b..c09e2d7040 100644 --- 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() +` without replicating +the file path validations existing in the parent class, allowed for potential +directory-traversal via certain inputs when calling :meth:`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 new file mode 100644 index 0000000000..712d3ba2e2 --- /dev/null +++ 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 index fc3533ab7d..38d87dc7f2 100644 --- a/tests/file_storage/tests.py +++ b/tests/file_storage/tests.py @@ -288,22 +288,17 @@ class FileStorageTests(SimpleTestCase): 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 index 6db8f9022d..c46f5a490b 100644 --- a/tests/file_uploads/tests.py +++ b/tests/file_uploads/tests.py @@ -880,7 +880,7 @@ class DirectoryCreationTests(SimpleTestCase): 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):