Visitar URL original
[3.1.x] Fixed #32718 -- Relaxed file name validation in FileField. · django/django@b7d4a6f · GitHub
Skip to content

Commit b7d4a6f

Browse files
felixxmfelixxm
committed
[3.1.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 9fb9944 commit b7d4a6f

7 files changed

Lines changed: 134 additions & 16 deletions

File tree

‎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
@@ -319,12 +319,12 @@ def generate_filename(self, instance, filename):
319319
Until the storage layer, all file paths are expected to be Unix style
320320
(with forward slashes).
321321
"""
322-
filename = validate_file_name(filename)
323322
if callable(self.upload_to):
324323
filename = self.upload_to(instance, filename)
325324
else:
326325
dirname = datetime.datetime.now().strftime(str(self.upload_to))
327326
filename = posixpath.join(dirname, filename)
327+
filename = validate_file_name(filename, allow_relative_path=True)
328328
return self.storage.generate_filename(filename)
329329

330330
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/3.1.11.txt‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
===========================
2+
Django 3.1.11 release notes
3+
===========================
4+
5+
*May 13, 2021*
6+
7+
Django 3.1.11 fixes a regression in 3.1.9.
8+
9+
Bugfixes
10+
========
11+
12+
* Fixed a regression in Django 3.1.9 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: 2 additions & 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+
3.1.11
2829
3.1.10
2930
3.1.9
3031
3.1.8
@@ -63,6 +64,7 @@ versions of the documentation contain the release notes for any later releases.
6364
.. toctree::
6465
:maxdepth: 1
6566

67+
2.2.23
6668
2.2.22
6769
2.2.21
6870
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: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import unittest
66
from pathlib import Path
77

8+
from django.core.exceptions import SuspiciousFileOperation
89
from django.core.files import File, temp
910
from django.core.files.base import ContentFile
1011
from django.core.files.uploadedfile import TemporaryUploadedFile
@@ -62,6 +63,15 @@ def test_refresh_from_db(self):
6263
d.refresh_from_db()
6364
self.assertIs(d.myfile.instance, d)
6465

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

0 commit comments

Comments
 (0)