181 lines
7.9 KiB
Diff
181 lines
7.9 KiB
Diff
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] [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/base.py | 11 +++++
|
|
django/core/files/utils.py | 7 ++--
|
|
tests/file_storage/test_base.py | 70 +++++++++++++++++++++++++++++++
|
|
tests/file_storage/tests.py | 11 ++---
|
|
tests/file_uploads/tests.py | 2 +-
|
|
5 files changed, 88 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 16ac22f70a..03a1b44edb 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 85342b2f3f..11e4f07724 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..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 7fb57fbce4..44bea8c180 100644
|
|
--- a/tests/file_storage/tests.py
|
|
+++ b/tests/file_storage/tests.py
|
|
@@ -342,22 +342,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 693efc4c62..24c703a309 100644
|
|
--- a/tests/file_uploads/tests.py
|
|
+++ b/tests/file_uploads/tests.py
|
|
@@ -826,7 +826,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):
|
|
--
|
|
2.45.2
|
|
|