File CVE-2024-39330.patch of Package python-Django.34811
From 354c91df0cb1027a403cf6888c59df5b7f414eaf Mon Sep 17 00:00:00 2001
From: nkrapp <nico.krapp@suse.com>
Date: Mon, 15 Jul 2024 14:00:13 +0200
Subject: [PATCH] [4.2.x] 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.py | 16 +++++++-
django/core/files/utils.py | 25 ++++++++++++
tests/file_storage/test_base.py | 70 +++++++++++++++++++++++++++++++++
tests/file_storage/tests.py | 20 ++++++++--
4 files changed, 127 insertions(+), 4 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 30788d6d75..ec35252a26 100644
--- a/django/core/files/storage.py
+++ b/django/core/files/storage.py
@@ -45,8 +45,22 @@ 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)
- return self._save(name, content)
+ # 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)
+ return name
# These methods are part of the public API, with default implementations.
diff --git a/django/core/files/utils.py b/django/core/files/utils.py
index de89607175..89c54bfa10 100644
--- a/django/core/files/utils.py
+++ b/django/core/files/utils.py
@@ -1,3 +1,28 @@
+import os
+import pathlib
+
+from django.core.exceptions import SuspiciousFileOperation
+
+
+def validate_file_name(name, allow_relative_path=False):
+ # Remove potentially dangerous names
+ if os.path.basename(name) in {"", ".", ".."}:
+ raise SuspiciousFileOperation("Could not derive file name from '%s'" % name)
+
+ if allow_relative_path:
+ # 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
+ )
+ elif name != os.path.basename(name):
+ raise SuspiciousFileOperation("File name '%s' includes path elements" % name)
+
+ return name
+
+
class FileProxyMixin:
"""
A mixin class used to forward file methods to an underlaying file
diff --git a/tests/file_storage/test_base.py b/tests/file_storage/test_base.py
new file mode 100644
index 0000000000..c5338b8e66
--- /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 3d232bb176..f2fde1e7bd 100644
--- a/tests/file_storage/tests.py
+++ b/tests/file_storage/tests.py
@@ -285,10 +285,24 @@ class FileStorageTests(SimpleTestCase):
with self.storage.open('path/to/test.file') as f:
self.assertEqual(f.read(), b'file saved with path')
- self.assertTrue(os.path.exists(
- os.path.join(self.temp_dir, 'path', 'to', 'test.file')))
+ self.assertTrue(
+ os.path.exists(os.path.join(self.temp_dir, "path", "to", "test.file"))
+ )
+
+ self.storage.delete("path/to/test.file")
- self.storage.delete('path/to/test.file')
+ @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_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_file_name, f)
+ self.assertIs(os.path.exists(os.path.join(self.temp_dir, f_name)), True)
def test_save_doesnt_close(self):
with TemporaryUploadedFile('test', 'text/plain', 1, 'utf8') as file:
--
2.45.2
