Skip to content

Commit b8ecb06

Browse files
felixxmfelixxm
committed
[2.2.x] Fixed #32718 -- Relaxed file name validation in FileField.
- Validate filename returned by FileField.upload_to() not a filename passed to the FileField.generate_filename() (upload_to() may completely ignored passed filename). - Allow relative paths (without dot segments) in the generated filename. Thanks to Jakub Kleň for the report and review. Thanks to all folks for checking this patch on existing projects. Thanks Florian Apolloner and Markus Holtermann for the discussion and implementation idea. Regression in 0b79eb3. Backport of b556999 from main.
1 parent 3ba089a commit b8ecb06

File tree

6 files changed

+120
-17
lines changed

6 files changed

+120
-17
lines changed

‎django/core/files/utils.py‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,26 @@
11
import os
2+
import pathlib
23

34
from django.core.exceptions import SuspiciousFileOperation
45

56

6-
def validate_file_name(name):
7-
if name != os.path.basename(name):
8-
raise SuspiciousFileOperation("File name '%s' includes path elements" % name)
9-
7+
def validate_file_name(name, allow_relative_path=False):
108
# Remove potentially dangerous names
11-
if name in {'', '.', '..'}:
9+
if os.path.basename(name) in {'', '.', '..'}:
1210
raise SuspiciousFileOperation("Could not derive file name from '%s'" % name)
1311

12+
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)
17+
if path.is_absolute() or '..' in path.parts:
18+
raise SuspiciousFileOperation(
19+
"Detected path traversal attempt in '%s'" % name
20+
)
21+
elif name != os.path.basename(name):
22+
raise SuspiciousFileOperation("File name '%s' includes path elements" % name)
23+
1424
return name
1525

1626

‎django/db/models/fields/files.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -300,12 +300,12 @@ def generate_filename(self, instance, filename):
300300
Until the storage layer, all file paths are expected to be Unix style
301301
(with forward slashes).
302302
"""
303-
filename = validate_file_name(filename)
304303
if callable(self.upload_to):
305304
filename = self.upload_to(instance, filename)
306305
else:
307306
dirname = datetime.datetime.now().strftime(self.upload_to)
308307
filename = posixpath.join(dirname, filename)
308+
filename = validate_file_name(filename, allow_relative_path=True)
309309
return self.storage.generate_filename(filename)
310310

311311
def save_form_data(self, instance, data):

‎docs/releases/2.2.23.txt‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
===========================
2+
Django 2.2.23 release notes
3+
===========================
4+
5+
*May 13, 2021*
6+
7+
Django 2.2.23 fixes a regression in 2.2.21.
8+
9+
Bugfixes
10+
========
11+
12+
* Fixed a regression in Django 2.2.21 where saving ``FileField`` would raise a
13+
``SuspiciousFileOperation`` even when a custom
14+
:attr:`~django.db.models.FileField.upload_to` returns a valid file path
15+
(:ticket:`32718`).

‎docs/releases/index.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ versions of the documentation contain the release notes for any later releases.
2525
.. toctree::
2626
:maxdepth: 1
2727

28+
2.2.23
2829
2.2.22
2930
2.2.21
3031
2.2.20

‎tests/file_storage/test_generate_filename.py‎

Lines changed: 76 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,4 @@
11
import os
2-
import sys
3-
from unittest import skipIf
42

53
from django.core.exceptions import SuspiciousFileOperation
64
from django.core.files.base import ContentFile
@@ -64,19 +62,37 @@ def test_storage_dangerous_paths_dir_name(self):
6462
s.generate_filename(file_name)
6563

6664
def test_filefield_dangerous_filename(self):
67-
candidates = ['..', '.', '', '???', '$.$.$']
65+
candidates = [
66+
('..', 'some/folder/..'),
67+
('.', 'some/folder/.'),
68+
('', 'some/folder/'),
69+
('???', '???'),
70+
('$.$.$', '$.$.$'),
71+
]
6872
f = FileField(upload_to='some/folder/')
69-
msg = "Could not derive file name from '%s'"
70-
for file_name in candidates:
73+
for file_name, msg_file_name in candidates:
74+
msg = f"Could not derive file name from '{msg_file_name}'"
7175
with self.subTest(file_name=file_name):
72-
with self.assertRaisesMessage(SuspiciousFileOperation, msg % file_name):
76+
with self.assertRaisesMessage(SuspiciousFileOperation, msg):
7377
f.generate_filename(None, file_name)
7478

75-
def test_filefield_dangerous_filename_dir(self):
79+
def test_filefield_dangerous_filename_dot_segments(self):
7680
f = FileField(upload_to='some/folder/')
77-
msg = "File name '/tmp/path' includes path elements"
81+
msg = "Detected path traversal attempt in 'some/folder/../path'"
7882
with self.assertRaisesMessage(SuspiciousFileOperation, msg):
79-
f.generate_filename(None, '/tmp/path')
83+
f.generate_filename(None, '../path')
84+
85+
def test_filefield_generate_filename_absolute_path(self):
86+
f = FileField(upload_to='some/folder/')
87+
candidates = [
88+
'/tmp/path',
89+
'/tmp/../path',
90+
]
91+
for file_name in candidates:
92+
msg = f"Detected path traversal attempt in '{file_name}'"
93+
with self.subTest(file_name=file_name):
94+
with self.assertRaisesMessage(SuspiciousFileOperation, msg):
95+
f.generate_filename(None, file_name)
8096

8197
def test_filefield_generate_filename(self):
8298
f = FileField(upload_to='some/folder/')
@@ -95,7 +111,57 @@ def upload_to(instance, filename):
95111
os.path.normpath('some/folder/test_with_space.txt')
96112
)
97113

98-
@skipIf(sys.platform == 'win32', 'Path components in filename are not supported after 0b79eb3.')
114+
def test_filefield_generate_filename_upload_to_overrides_dangerous_filename(self):
115+
def upload_to(instance, filename):
116+
return 'test.txt'
117+
118+
f = FileField(upload_to=upload_to)
119+
candidates = [
120+
'/tmp/.',
121+
'/tmp/..',
122+
'/tmp/../path',
123+
'/tmp/path',
124+
'some/folder/',
125+
'some/folder/.',
126+
'some/folder/..',
127+
'some/folder/???',
128+
'some/folder/$.$.$',
129+
'some/../test.txt',
130+
'',
131+
]
132+
for file_name in candidates:
133+
with self.subTest(file_name=file_name):
134+
self.assertEqual(f.generate_filename(None, file_name), 'test.txt')
135+
136+
def test_filefield_generate_filename_upload_to_absolute_path(self):
137+
def upload_to(instance, filename):
138+
return '/tmp/' + filename
139+
140+
f = FileField(upload_to=upload_to)
141+
candidates = [
142+
'path',
143+
'../path',
144+
'???',
145+
'$.$.$',
146+
]
147+
for file_name in candidates:
148+
msg = f"Detected path traversal attempt in '/tmp/{file_name}'"
149+
with self.subTest(file_name=file_name):
150+
with self.assertRaisesMessage(SuspiciousFileOperation, msg):
151+
f.generate_filename(None, file_name)
152+
153+
def test_filefield_generate_filename_upload_to_dangerous_filename(self):
154+
def upload_to(instance, filename):
155+
return '/tmp/' + filename
156+
157+
f = FileField(upload_to=upload_to)
158+
candidates = ['..', '.', '']
159+
for file_name in candidates:
160+
msg = f"Could not derive file name from '/tmp/{file_name}'"
161+
with self.subTest(file_name=file_name):
162+
with self.assertRaisesMessage(SuspiciousFileOperation, msg):
163+
f.generate_filename(None, file_name)
164+
99165
def test_filefield_awss3_storage(self):
100166
"""
101167
Simulate a FileField with an S3 storage which uses keys rather than

‎tests/model_fields/test_filefield.py‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
import os
22
import sys
3+
import tempfile
34
import unittest
45

5-
from django.core.files import temp
6+
from django.core.exceptions import SuspiciousFileOperation
7+
from django.core.files import File, temp
68
from django.core.files.base import ContentFile
79
from django.core.files.uploadedfile import TemporaryUploadedFile
810
from django.db.utils import IntegrityError
@@ -59,6 +61,15 @@ def test_refresh_from_db(self):
5961
d.refresh_from_db()
6062
self.assertIs(d.myfile.instance, d)
6163

64+
@unittest.skipIf(sys.platform == 'win32', "Crashes with OSError on Windows.")
65+
def test_save_without_name(self):
66+
with tempfile.NamedTemporaryFile(suffix='.txt') as tmp:
67+
document = Document.objects.create(myfile='something.txt')
68+
document.myfile = File(tmp)
69+
msg = f"Detected path traversal attempt in '{tmp.name}'"
70+
with self.assertRaisesMessage(SuspiciousFileOperation, msg):
71+
document.save()
72+
6273
def test_defer(self):
6374
Document.objects.create(myfile='something.txt')
6475
self.assertEqual(Document.objects.defer('myfile')[0].myfile, 'something.txt')

0 commit comments

Comments
 (0)