Skip to content

Commit 25d84d6

Browse files
apollo13carltongibson
authored andcommitted
[3.1.x] Fixed CVE-2021-31542 -- Tightened path & file name sanitation in file uploads.
1 parent 6b0c7e6 commit 25d84d6

File tree

13 files changed

+178
-11
lines changed

13 files changed

+178
-11
lines changed

Diff for: django/core/files/storage.py

+7
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
import os
2+
import pathlib
23
from datetime import datetime
34
from urllib.parse import urljoin
45

56
from django.conf import settings
67
from django.core.exceptions import SuspiciousFileOperation
78
from django.core.files import File, locks
89
from django.core.files.move import file_move_safe
10+
from django.core.files.utils import validate_file_name
911
from django.core.signals import setting_changed
1012
from django.utils import timezone
1113
from django.utils._os import safe_join
@@ -74,6 +76,9 @@ def get_available_name(self, name, max_length=None):
7476
available for new content to be written to.
7577
"""
7678
dir_name, file_name = os.path.split(name)
79+
if '..' in pathlib.PurePath(dir_name).parts:
80+
raise SuspiciousFileOperation("Detected path traversal attempt in '%s'" % dir_name)
81+
validate_file_name(file_name)
7782
file_root, file_ext = os.path.splitext(file_name)
7883
# If the filename already exists, generate an alternative filename
7984
# until it doesn't exist.
@@ -105,6 +110,8 @@ def generate_filename(self, filename):
105110
"""
106111
# `filename` may include a path as returned by FileField.upload_to.
107112
dirname, filename = os.path.split(filename)
113+
if '..' in pathlib.PurePath(dirname).parts:
114+
raise SuspiciousFileOperation("Detected path traversal attempt in '%s'" % dirname)
108115
return os.path.normpath(os.path.join(dirname, self.get_valid_name(filename)))
109116

110117
def path(self, name):

Diff for: django/core/files/uploadedfile.py

+3
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
from django.conf import settings
99
from django.core.files import temp as tempfile
1010
from django.core.files.base import File
11+
from django.core.files.utils import validate_file_name
1112

1213
__all__ = ('UploadedFile', 'TemporaryUploadedFile', 'InMemoryUploadedFile',
1314
'SimpleUploadedFile')
@@ -47,6 +48,8 @@ def _set_name(self, name):
4748
ext = ext[:255]
4849
name = name[:255 - len(ext)] + ext
4950

51+
name = validate_file_name(name)
52+
5053
self._name = name
5154

5255
name = property(_get_name, _set_name)

Diff for: django/core/files/utils.py

+16
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,19 @@
1+
import os
2+
3+
from django.core.exceptions import SuspiciousFileOperation
4+
5+
6+
def validate_file_name(name):
7+
if name != os.path.basename(name):
8+
raise SuspiciousFileOperation("File name '%s' includes path elements" % name)
9+
10+
# Remove potentially dangerous names
11+
if name in {'', '.', '..'}:
12+
raise SuspiciousFileOperation("Could not derive file name from '%s'" % name)
13+
14+
return name
15+
16+
117
class FileProxyMixin:
218
"""
319
A mixin class used to forward file methods to an underlaying file

Diff for: django/db/models/fields/files.py

+2
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
from django.core.files.base import File
77
from django.core.files.images import ImageFile
88
from django.core.files.storage import Storage, default_storage
9+
from django.core.files.utils import validate_file_name
910
from django.db.models import signals
1011
from django.db.models.fields import Field
1112
from django.utils.translation import gettext_lazy as _
@@ -318,6 +319,7 @@ def generate_filename(self, instance, filename):
318319
Until the storage layer, all file paths are expected to be Unix style
319320
(with forward slashes).
320321
"""
322+
filename = validate_file_name(filename)
321323
if callable(self.upload_to):
322324
filename = self.upload_to(instance, filename)
323325
else:

Diff for: django/http/multipartparser.py

+18-4
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
import cgi
1010
import collections
1111
import html
12-
import os
1312
from urllib.parse import unquote
1413

1514
from django.conf import settings
@@ -299,10 +298,25 @@ def handle_file_complete(self, old_field_name, counters):
299298
break
300299

301300
def sanitize_file_name(self, file_name):
301+
"""
302+
Sanitize the filename of an upload.
303+
304+
Remove all possible path separators, even though that might remove more
305+
than actually required by the target system. Filenames that could
306+
potentially cause problems (current/parent dir) are also discarded.
307+
308+
It should be noted that this function could still return a "filepath"
309+
like "C:some_file.txt" which is handled later on by the storage layer.
310+
So while this function does sanitize filenames to some extent, the
311+
resulting filename should still be considered as untrusted user input.
312+
"""
302313
file_name = html.unescape(file_name)
303-
# Cleanup Windows-style path separators.
304-
file_name = file_name[file_name.rfind('\\') + 1:].strip()
305-
return os.path.basename(file_name)
314+
file_name = file_name.rsplit('/')[-1]
315+
file_name = file_name.rsplit('\\')[-1]
316+
317+
if file_name in {'', '.', '..'}:
318+
return None
319+
return file_name
306320

307321
IE_sanitize = sanitize_file_name
308322

Diff for: django/utils/text.py

+7-3
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
from gzip import GzipFile
66
from io import BytesIO
77

8+
from django.core.exceptions import SuspiciousFileOperation
89
from django.utils.deprecation import RemovedInDjango40Warning
910
from django.utils.functional import SimpleLazyObject, keep_lazy_text, lazy
1011
from django.utils.regex_helper import _lazy_re_compile
@@ -219,7 +220,7 @@ def _truncate_html(self, length, truncate, text, truncate_len, words):
219220

220221

221222
@keep_lazy_text
222-
def get_valid_filename(s):
223+
def get_valid_filename(name):
223224
"""
224225
Return the given string converted to a string that can be used for a clean
225226
filename. Remove leading and trailing spaces; convert other spaces to
@@ -228,8 +229,11 @@ def get_valid_filename(s):
228229
>>> get_valid_filename("john's portrait in 2004.jpg")
229230
'johns_portrait_in_2004.jpg'
230231
"""
231-
s = str(s).strip().replace(' ', '_')
232-
return re.sub(r'(?u)[^-\w.]', '', s)
232+
s = str(name).strip().replace(' ', '_')
233+
s = re.sub(r'(?u)[^-\w.]', '', s)
234+
if s in {'', '.', '..'}:
235+
raise SuspiciousFileOperation("Could not derive file name from '%s'" % name)
236+
return s
233237

234238

235239
@keep_lazy_text

Diff for: docs/releases/2.2.21.txt

+17
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
===========================
2+
Django 2.2.21 release notes
3+
===========================
4+
5+
*May 4, 2021*
6+
7+
Django 2.2.21 fixes a security issue in 2.2.20.
8+
9+
CVE-2021-31542: Potential directory-traversal via uploaded files
10+
================================================================
11+
12+
``MultiPartParser``, ``UploadedFile``, and ``FieldFile`` allowed
13+
directory-traversal via uploaded files with suitably crafted file names.
14+
15+
In order to mitigate this risk, stricter basename and path sanitation is now
16+
applied. Specifically, empty file names and paths with dot segments will be
17+
rejected.

Diff for: docs/releases/3.1.9.txt

+17
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
==========================
2+
Django 3.1.9 release notes
3+
==========================
4+
5+
*May 4, 2021*
6+
7+
Django 3.1.9 fixes a security issue in 3.1.8.
8+
9+
CVE-2021-31542: Potential directory-traversal via uploaded files
10+
================================================================
11+
12+
``MultiPartParser``, ``UploadedFile``, and ``FieldFile`` allowed
13+
directory-traversal via uploaded files with suitably crafted file names.
14+
15+
In order to mitigate this risk, stricter basename and path sanitation is now
16+
applied. Specifically, empty file names and paths with dot segments will be
17+
rejected.

Diff for: docs/releases/index.txt

+2
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.9
2829
3.1.8
2930
3.1.7
3031
3.1.6
@@ -61,6 +62,7 @@ versions of the documentation contain the release notes for any later releases.
6162
.. toctree::
6263
:maxdepth: 1
6364

65+
2.2.21
6466
2.2.20
6567
2.2.19
6668
2.2.18

Diff for: tests/file_storage/test_generate_filename.py

+40-1
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
import os
22

3+
from django.core.exceptions import SuspiciousFileOperation
34
from django.core.files.base import ContentFile
4-
from django.core.files.storage import Storage
5+
from django.core.files.storage import FileSystemStorage, Storage
56
from django.db.models import FileField
67
from django.test import SimpleTestCase
78

@@ -36,6 +37,44 @@ def generate_filename(self, filename):
3637

3738

3839
class GenerateFilenameStorageTests(SimpleTestCase):
40+
def test_storage_dangerous_paths(self):
41+
candidates = [
42+
('/tmp/..', '..'),
43+
('/tmp/.', '.'),
44+
('', ''),
45+
]
46+
s = FileSystemStorage()
47+
msg = "Could not derive file name from '%s'"
48+
for file_name, base_name in candidates:
49+
with self.subTest(file_name=file_name):
50+
with self.assertRaisesMessage(SuspiciousFileOperation, msg % base_name):
51+
s.get_available_name(file_name)
52+
with self.assertRaisesMessage(SuspiciousFileOperation, msg % base_name):
53+
s.generate_filename(file_name)
54+
55+
def test_storage_dangerous_paths_dir_name(self):
56+
file_name = '/tmp/../path'
57+
s = FileSystemStorage()
58+
msg = "Detected path traversal attempt in '/tmp/..'"
59+
with self.assertRaisesMessage(SuspiciousFileOperation, msg):
60+
s.get_available_name(file_name)
61+
with self.assertRaisesMessage(SuspiciousFileOperation, msg):
62+
s.generate_filename(file_name)
63+
64+
def test_filefield_dangerous_filename(self):
65+
candidates = ['..', '.', '', '???', '$.$.$']
66+
f = FileField(upload_to='some/folder/')
67+
msg = "Could not derive file name from '%s'"
68+
for file_name in candidates:
69+
with self.subTest(file_name=file_name):
70+
with self.assertRaisesMessage(SuspiciousFileOperation, msg % file_name):
71+
f.generate_filename(None, file_name)
72+
73+
def test_filefield_dangerous_filename_dir(self):
74+
f = FileField(upload_to='some/folder/')
75+
msg = "File name '/tmp/path' includes path elements"
76+
with self.assertRaisesMessage(SuspiciousFileOperation, msg):
77+
f.generate_filename(None, '/tmp/path')
3978

4079
def test_filefield_generate_filename(self):
4180
f = FileField(upload_to='some/folder/')

Diff for: tests/file_uploads/tests.py

+37-1
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,9 @@
88
from io import BytesIO, StringIO
99
from urllib.parse import quote
1010

11+
from django.core.exceptions import SuspiciousFileOperation
1112
from django.core.files import temp as tempfile
12-
from django.core.files.uploadedfile import SimpleUploadedFile
13+
from django.core.files.uploadedfile import SimpleUploadedFile, UploadedFile
1314
from django.http.multipartparser import (
1415
MultiPartParser, MultiPartParserError, parse_header,
1516
)
@@ -38,6 +39,16 @@
3839
'../hax0rd.txt', # HTML entities.
3940
]
4041

42+
CANDIDATE_INVALID_FILE_NAMES = [
43+
'/tmp/', # Directory, *nix-style.
44+
'c:\\tmp\\', # Directory, win-style.
45+
'/tmp/.', # Directory dot, *nix-style.
46+
'c:\\tmp\\.', # Directory dot, *nix-style.
47+
'/tmp/..', # Parent directory, *nix-style.
48+
'c:\\tmp\\..', # Parent directory, win-style.
49+
'', # Empty filename.
50+
]
51+
4152

4253
@override_settings(MEDIA_ROOT=MEDIA_ROOT, ROOT_URLCONF='file_uploads.urls', MIDDLEWARE=[])
4354
class FileUploadTests(TestCase):
@@ -52,6 +63,22 @@ def tearDownClass(cls):
5263
shutil.rmtree(MEDIA_ROOT)
5364
super().tearDownClass()
5465

66+
def test_upload_name_is_validated(self):
67+
candidates = [
68+
'/tmp/',
69+
'/tmp/..',
70+
'/tmp/.',
71+
]
72+
if sys.platform == 'win32':
73+
candidates.extend([
74+
'c:\\tmp\\',
75+
'c:\\tmp\\..',
76+
'c:\\tmp\\.',
77+
])
78+
for file_name in candidates:
79+
with self.subTest(file_name=file_name):
80+
self.assertRaises(SuspiciousFileOperation, UploadedFile, name=file_name)
81+
5582
def test_simple_upload(self):
5683
with open(__file__, 'rb') as fp:
5784
post_data = {
@@ -685,6 +712,15 @@ def test_sanitize_file_name(self):
685712
with self.subTest(file_name=file_name):
686713
self.assertEqual(parser.sanitize_file_name(file_name), 'hax0rd.txt')
687714

715+
def test_sanitize_invalid_file_name(self):
716+
parser = MultiPartParser({
717+
'CONTENT_TYPE': 'multipart/form-data; boundary=_foo',
718+
'CONTENT_LENGTH': '1',
719+
}, StringIO('x'), [], 'utf-8')
720+
for file_name in CANDIDATE_INVALID_FILE_NAMES:
721+
with self.subTest(file_name=file_name):
722+
self.assertIsNone(parser.sanitize_file_name(file_name))
723+
688724
def test_rfc2231_parsing(self):
689725
test_data = (
690726
(b"Content-Type: application/x-stuff; title*=us-ascii'en-us'This%20is%20%2A%2A%2Afun%2A%2A%2A",

Diff for: tests/forms_tests/field_tests/test_filefield.py

+4-2
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,12 @@ def test_filefield_1(self):
2121
f.clean(None, '')
2222
self.assertEqual('files/test2.pdf', f.clean(None, 'files/test2.pdf'))
2323
no_file_msg = "'No file was submitted. Check the encoding type on the form.'"
24+
file = SimpleUploadedFile(None, b'')
25+
file._name = ''
2426
with self.assertRaisesMessage(ValidationError, no_file_msg):
25-
f.clean(SimpleUploadedFile('', b''))
27+
f.clean(file)
2628
with self.assertRaisesMessage(ValidationError, no_file_msg):
27-
f.clean(SimpleUploadedFile('', b''), '')
29+
f.clean(file, '')
2830
self.assertEqual('files/test3.pdf', f.clean(None, 'files/test3.pdf'))
2931
with self.assertRaisesMessage(ValidationError, no_file_msg):
3032
f.clean('some content that is not a file')

Diff for: tests/utils_tests/test_text.py

+8
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import json
22
import sys
33

4+
from django.core.exceptions import SuspiciousFileOperation
45
from django.test import SimpleTestCase, ignore_warnings
56
from django.utils import text
67
from django.utils.deprecation import RemovedInDjango40Warning
@@ -243,6 +244,13 @@ def test_get_valid_filename(self):
243244
filename = "^&'@{}[],$=!-#()%+~_123.txt"
244245
self.assertEqual(text.get_valid_filename(filename), "-_123.txt")
245246
self.assertEqual(text.get_valid_filename(lazystr(filename)), "-_123.txt")
247+
msg = "Could not derive file name from '???'"
248+
with self.assertRaisesMessage(SuspiciousFileOperation, msg):
249+
text.get_valid_filename('???')
250+
# After sanitizing this would yield '..'.
251+
msg = "Could not derive file name from '$.$.$'"
252+
with self.assertRaisesMessage(SuspiciousFileOperation, msg):
253+
text.get_valid_filename('$.$.$')
246254

247255
def test_compress_sequence(self):
248256
data = [{'key': i} for i in range(10)]

0 commit comments

Comments
 (0)