From 2b00edc0151a660d1eb86da4059904a0fc4e095e 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. CVE: CVE-2024-39330 Upstream-Status: Backport https://github.com/django/django/commit/2b00edc0151a660d1eb86da4059904a0fc4e095e Signed-off-by: Saravanan --- django/core/files/storage.py | 11 ++++++ django/core/files/utils.py | 7 ++-- docs/releases/2.2.28.txt | 12 ++++++ tests/file_storage/test_base.py | 70 +++++++++++++++++++++++++++++++++ tests/file_storage/tests.py | 6 --- 5 files changed, 96 insertions(+), 10 deletions(-) create mode 100644 tests/file_storage/test_base.py diff --git a/django/core/files/storage.py b/django/core/files/storage.py index ea5bbc8..8c633ec 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 f28cea1..a1fea44 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/2.2.28.txt b/docs/releases/2.2.28.txt index 22fa80e..3503f38 100644 --- a/docs/releases/2.2.28.txt +++ b/docs/releases/2.2.28.txt @@ -131,3 +131,15 @@ 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/tests/file_storage/test_base.py b/tests/file_storage/test_base.py new file mode 100644 index 0000000..c5338b8 --- /dev/null +++ b/tests/file_storage/test_base.py @@ -0,0 +1,70 @@ +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 4c6f692..0e69264 100644 --- a/tests/file_storage/tests.py +++ b/tests/file_storage/tests.py @@ -291,12 +291,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') -- 2.48.1