Skip to content

Commit 0b79eb3

Browse files
apollo13carltongibson
apollo13
authored and
carltongibson
committed
Fixed CVE-2021-31542 -- Tightened path & file name sanitation in file uploads.
1 parent 8de4ca7 commit 0b79eb3

File tree

14 files changed

+190
-13
lines changed

14 files changed

+190
-13
lines changed

‎django/core/files/storage.py

Lines changed: 7 additions & 0 deletions
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):

‎django/core/files/uploadedfile.py

Lines changed: 3 additions & 0 deletions
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)

‎django/core/files/utils.py

Lines changed: 16 additions & 0 deletions
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

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

Lines changed: 2 additions & 0 deletions
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.db.models.query_utils import DeferredAttribute
@@ -312,6 +313,7 @@ def generate_filename(self, instance, filename):
312313
Until the storage layer, all file paths are expected to be Unix style
313314
(with forward slashes).
314315
"""
316+
filename = validate_file_name(filename)
315317
if callable(self.upload_to):
316318
filename = self.upload_to(instance, filename)
317319
else:

‎django/http/multipartparser.py

Lines changed: 18 additions & 4 deletions
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
@@ -306,10 +305,25 @@ def handle_file_complete(self, old_field_name, counters):
306305
break
307306

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

314328
IE_sanitize = sanitize_file_name
315329

‎django/utils/text.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
from gzip import GzipFile
55
from io import BytesIO
66

7+
from django.core.exceptions import SuspiciousFileOperation
78
from django.utils.functional import SimpleLazyObject, keep_lazy_text, lazy
89
from django.utils.regex_helper import _lazy_re_compile
910
from django.utils.translation import gettext as _, gettext_lazy, pgettext
@@ -221,7 +222,7 @@ def _truncate_html(self, length, truncate, text, truncate_len, words):
221222

222223

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

236240

237241
@keep_lazy_text

‎docs/releases/2.2.21.txt

Lines changed: 17 additions & 0 deletions
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.

‎docs/releases/3.1.9.txt

Lines changed: 17 additions & 0 deletions
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.

‎docs/releases/3.2.1.txt

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,19 @@
22
Django 3.2.1 release notes
33
==========================
44

5-
*Expected May 4, 2021*
5+
*May 4, 2021*
66

7-
Django 3.2.1 fixes several bugs in 3.2.
7+
Django 3.2.1 fixes a security issue and several bugs in 3.2.
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.
818

919
Bugfixes
1020
========

‎docs/releases/index.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ versions of the documentation contain the release notes for any later releases.
4040
.. toctree::
4141
:maxdepth: 1
4242

43+
3.1.9
4344
3.1.8
4445
3.1.7
4546
3.1.6
@@ -76,6 +77,7 @@ versions of the documentation contain the release notes for any later releases.
7677
.. toctree::
7778
:maxdepth: 1
7879

80+
2.2.21
7981
2.2.20
8082
2.2.19
8183
2.2.18

‎tests/file_storage/test_generate_filename.py

Lines changed: 40 additions & 1 deletion
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/')

‎tests/file_uploads/tests.py

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,9 @@
99
from unittest import mock
1010
from urllib.parse import quote
1111

12+
from django.core.exceptions import SuspiciousFileOperation
1213
from django.core.files import temp as tempfile
13-
from django.core.files.uploadedfile import SimpleUploadedFile
14+
from django.core.files.uploadedfile import SimpleUploadedFile, UploadedFile
1415
from django.http.multipartparser import (
1516
FILE, MultiPartParser, MultiPartParserError, Parser, parse_header,
1617
)
@@ -39,6 +40,16 @@
3940
'../hax0rd.txt', # HTML entities.
4041
]
4142

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

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

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

748+
def test_sanitize_invalid_file_name(self):
749+
parser = MultiPartParser({
750+
'CONTENT_TYPE': 'multipart/form-data; boundary=_foo',
751+
'CONTENT_LENGTH': '1',
752+
}, StringIO('x'), [], 'utf-8')
753+
for file_name in CANDIDATE_INVALID_FILE_NAMES:
754+
with self.subTest(file_name=file_name):
755+
self.assertIsNone(parser.sanitize_file_name(file_name))
756+
721757
def test_rfc2231_parsing(self):
722758
test_data = (
723759
(b"Content-Type: application/x-stuff; title*=us-ascii'en-us'This%20is%20%2A%2A%2Afun%2A%2A%2A",

‎tests/forms_tests/field_tests/test_filefield.py

Lines changed: 4 additions & 2 deletions
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')

‎tests/utils_tests/test_text.py

Lines changed: 8 additions & 0 deletions
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
56
from django.utils import text
67
from django.utils.functional import lazystr
@@ -228,6 +229,13 @@ def test_get_valid_filename(self):
228229
filename = "^&'@{}[],$=!-#()%+~_123.txt"
229230
self.assertEqual(text.get_valid_filename(filename), "-_123.txt")
230231
self.assertEqual(text.get_valid_filename(lazystr(filename)), "-_123.txt")
232+
msg = "Could not derive file name from '???'"
233+
with self.assertRaisesMessage(SuspiciousFileOperation, msg):
234+
text.get_valid_filename('???')
235+
# After sanitizing this would yield '..'.
236+
msg = "Could not derive file name from '$.$.$'"
237+
with self.assertRaisesMessage(SuspiciousFileOperation, msg):
238+
text.get_valid_filename('$.$.$')
231239

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

0 commit comments

Comments
 (0)