commit fe4a0bbe2088d0c2b331216dad21ccd0bb3ee80d Author: Natalia <124304+nessita@users.noreply.github.com> Date: Wed Mar 20 13:55:21 2024 -0300 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. diff --git a/django/core/files/storage.py b/django/core/files/storage.py index 22984f949..680f5ec91 100644 --- a/django/core/files/storage.py +++ b/django/core/files/storage.py @@ -50,7 +50,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/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 723809324..6d17a7118 100644 --- a/tests/file_storage/tests.py +++ b/tests/file_storage/tests.py @@ -297,12 +297,6 @@ 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) - def test_save_doesnt_close(self): with TemporaryUploadedFile('test', 'text/plain', 1, 'utf8') as file: file.write(b'1')