1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
|
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 <saravanan.kadambathursubramaniyam@windriver.com>
---
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()
+<django.core.files.storage.Storage.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()
+<django.core.files.storage.Storage.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
|
