8000 [5.0.x] Fixed CVE-2024-39330 -- Added extra file name validation in S… · django/django@9f4f63e · GitHub
[go: up one dir, main page]
Skip to content

Commit 9f4f63e

Browse files
nessitanessita
committed
[5.0.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.
1 parent 07cefde commit 9f4f63e

File tree

7 files changed

+114
-13
lines changed

7 files changed

+114
-13
lines changed

‎django/core/files/storage/base.py‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,18 @@ def save(self, name, content, max_length=None):
3434
if not hasattr(content, "chunks"):
3535
content = File(content, name)
3636

37+
# Ensure that the name is valid, before and after having the storage
38+
# system potentially modifying the name. This duplicates the check made
39+
# inside `get_available_name` but it's necessary for those cases where
40+
# `get_available_name` is overriden and validation is lost.
41+
validate_file_name(name, allow_relative_path=True)
42+
43+
# Potentially find a different name depending on storage constraints.
3744
name = self.get_available_name(name, max_length=max_length)
45+
# Validate the (potentially) new name.
46+
validate_file_name(name, allow_relative_path=True)
47+
48+
# The save operation should return the actual name of the file saved.
3849
name = self._save(name, content)
3950
# Ensure that the name returned from the storage system is still valid.
4051
validate_file_name(name, allow_relative_path=True)

‎django/core/files/utils.py‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,9 @@ def validate_file_name(name, allow_relative_path=False):
1010
raise SuspiciousFileOperation("Could not derive file name from '%s'" % name)
1111

1212
if allow_relative_path:
13-
# Use PurePosixPath() because this branch is checked only in
14-
# FileField.generate_filename() where all file paths are expected to be
15-
# Unix style (with forward slashes).
16-
path = pathlib.PurePosixPath(name)
13+
# Ensure that name can be treated as a pure posix path, i.e. Unix
14+
# style (with forward slashes).
15+
path = pathlib.PurePosixPath(str(name).replace("\\", "/"))
1716
if path.is_absolute() or ".." in path.parts:
1817
raise SuspiciousFileOperation(
1918
"Detected path traversal attempt in '%s'" % name

‎docs/releases/4.2.14.txt‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,3 +20,15 @@ CVE-2024-39329: Username enumeration through timing difference for users with un
2020
The :meth:`~django.contrib.auth.backends.ModelBackend.authenticate()` method
2121
allowed remote attackers to enumerate users via a timing attack involving login
2222
requests for users with unusable passwords.
23+
24+
CVE-2024-39330: Potential directory-traversal via ``Storage.save()``
25+
====================================================================
26+
27+
Derived classes of the :class:`~django.core.files.storage.Storage` base class
28+
which override :meth:`generate_filename()
29+
<django.core.files.storage.Storage.generate_filename()>` without replicating
30+
the file path validations existing in the parent class, allowed for potential
31+
directory-traversal via certain inputs when calling :meth:`save()
32+
<django.core.files.storage.Storage.save()>`.
33+
34+
Built-in ``Storage`` sub-classes were not affected by this vulnerability.

‎docs/releases/5.0.7.txt‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,18 @@ The :meth:`~django.contrib.auth.backends.ModelBackend.authenticate()` method
2121
allowed remote attackers to enumerate users via a timing attack involving login
2222
requests for users with unusable passwords.
2323

24+
CVE-2024-39330: Potential directory-traversal via ``Storage.save()``
25+
====================================================================
26+
27+
Derived classes of the :class:`~django.core.files.storage.Storage` base class
28+
which override :meth:`generate_filename()
29+
<django.core.files.storage.Storage.generate_filename()>` without replicating
30+
the file path validations existing in the parent class, allowed for potential
31+
directory-traversal via certain inputs when calling :meth:`save()
32+
<django.core.files.storage.Storage.save()>`.
33+
34+
Built-in ``Storage`` sub-classes were not affected by this vulnerability.
35+
2436
Bugfixes
2537
========
2638

‎tests/file_storage/test_base.py‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
import os
2+
from unittest import mock
3+
4+
from django.core.exceptions import SuspiciousFileOperation
5+
from django.core.files.storage import Storage
6+
from django.test import SimpleTestCase
7+
8+
9+
class CustomStorage(Storage):
10+
"""Simple Storage subclass implementing the bare minimum for testing."""
11+
12+
def exists(self, name):
13+
return False
14+
15+
def _save(self, name):
16+
return name
17+
18+
19+
class StorageValidateFileNameTests(SimpleTestCase):
20+
21+
invalid_file_names = [
22+
os.path.join("path", "to", os.pardir, "test.file"),
23+
os.path.join(os.path.sep, "path", "to", "test.file"),
24+
]
25+
error_msg = "Detected path traversal attempt in '%s'"
26+
27+
def test_validate_before_get_available_name(self):
28+
s = CustomStorage()
29+
# The initial name passed to `save` is not valid nor safe, fail early.
30+
for name in self.invalid_file_names:
31+
with (
32+
self.subTest(name=name),
33+
mock.patch.object(s, "get_available_name") as mock_get_available_name,
34+
mock.patch.object(s, "_save") as mock_internal_save,
35+
):
36+
with self.assertRaisesMessage(
37+
SuspiciousFileOperation, self.error_msg % name
38+
):
39+
s.save(name, content="irrelevant")
40+
self.assertEqual(mock_get_available_name.mock_calls, [])
41+
self.assertEqual(mock_internal_save.mock_calls, [])
42+
43+
def test_validate_after_get_available_name(self):
44+
s = CustomStorage()
45+
# The initial name passed to `save` is valid and safe, but the returned
46+
# name from `get_available_name` is not.
47+
for name in self.invalid_file_names:
48+
with (
49+
self.subTest(name=name),
50+
mock.patch.object(s, "get_available_name", return_value=name),
51+
mock.patch.object(s, "_save") as mock_internal_save,
52+
):
53+
with self.assertRaisesMessage(
54+
SuspiciousFileOperation, self.error_msg % name
55+
):
56+
s.save("valid-file-name.txt", content="irrelevant")
57+
self.assertEqual(mock_internal_save.mock_calls, [])
58+
59+
def test_validate_after_internal_save(self):
60+
s = CustomStorage()
61+
# The initial name passed to `save` is valid and safe, but the result
62+
# from `_save` is not (this is achieved by monkeypatching _save).
63+
for name in self.invalid_file_names:
64+
with (
65+
self.subTest(name=name),
66+
mock.patch.object(s, "_save", return_value=name),
67+
):
68+
69+
with self.assertRaisesMessage(
70+
SuspiciousFileOperation, self.error_msg % name
71+
):
72+
s.save("valid-file-name.txt", content="irrelevant")

‎tests/file_storage/tests.py‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -342,22 +342,17 @@ def test_file_save_with_path(self):
342342

343343
self.storage.delete("path/to/test.file")
344344

345-
def test_file_save_abs_path(self):
346-
test_name = "path/to/test.file"
347-
f = ContentFile("file saved with path")
348-
f_name = self.storage.save(os.path.join(self.temp_dir, test_name), f)
349-
self.assertEqual(f_name, test_name)
350-
351345
@unittest.skipUnless(
352346
symlinks_supported(), "Must be able to symlink to run this test."
353347
)
354348
def test_file_save_broken_symlink(self):
355349
"""A new path is created on save when a broken symlink is supplied."""
356350
nonexistent_file_path = os.path.join(self.temp_dir, "nonexistent.txt")
357-
broken_symlink_path = os.path.join(self.temp_dir, "symlink.txt")
351+
broken_symlink_file_name = "symlink.txt"
352+
broken_symlink_path = os.path.join(self.temp_dir, broken_symlink_file_name)
358353
os.symlink(nonexistent_file_path, broken_symlink_path)
359354
f = ContentFile("some content")
360-
f_name = self.storage.save(broken_symlink_path, f)
355+
f_name = self.storage.save(broken_symlink_file_name, f)
361356
self.assertIs(os.path.exists(os.path.join(self.temp_dir, f_name)), True)
362357

363358
def test_save_doesnt_close(self):

‎tests/file_uploads/tests.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -828,7 +828,7 @@ def test_not_a_directory(self):
828828
default_storage.delete(UPLOAD_TO)
829829
# Create a file with the upload directory name
830830
with SimpleUploadedFile(UPLOAD_TO, b"x") as file:
831-
default_storage.save(UPLOAD_TO, file)
831+
default_storage.save(UPLOAD_FOLDER, file)
832832
self.addCleanup(default_storage.delete, UPLOAD_TO)
833833
msg = "%s exists and is not a directory." % UPLOAD_TO
834834
with self.assertRaisesMessage(FileExistsError, msg):

0 commit comments

Comments
 (0)
0