Skip to content

Commit 2820fd1

Browse files
felixxmfelixxm
committed
[3.2.x] Fixed CVE-2021-28658 -- Fixed potential directory-traversal via uploaded files.
Thanks Claude Paroz for the initial patch. Thanks Dennis Brinkrolf for the report. Backport of d4d800c from main.
1 parent eb7c0a7 commit 2820fd1

File tree

9 files changed

+159
-23
lines changed

9 files changed

+159
-23
lines changed

‎django/http/multipartparser.py‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -212,9 +212,8 @@ def parse(self):
212212
# This is a file, use the handler...
213213
file_name = disposition.get('filename')
214214
if file_name:
215-
file_name = os.path.basename(file_name)
216215
file_name = force_str(file_name, encoding, errors='replace')
217-
file_name = self.IE_sanitize(html.unescape(file_name))
216+
file_name = self.sanitize_file_name(file_name)
218217
if not file_name:
219218
continue
220219

@@ -306,9 +305,13 @@ def handle_file_complete(self, old_field_name, counters):
306305
self._files.appendlist(force_str(old_field_name, self._encoding, errors='replace'), file_obj)
307306
break
308307

309-
def IE_sanitize(self, filename):
310-
"""Cleanup filename from Internet Explorer full paths."""
311-
return filename and filename[filename.rfind("\\") + 1:].strip()
308+
def sanitize_file_name(self, file_name):
309+
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)
313+
314+
IE_sanitize = sanitize_file_name
312315

313316
def _close_files(self):
314317
# Free up all file handles.

‎docs/releases/2.2.20.txt‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
===========================
2+
Django 2.2.20 release notes
3+
===========================
4+
5+
*April 6, 2021*
6+
7+
Django 2.2.20 fixes a security issue with severity "low" in 2.2.19.
8+
9+
CVE-2021-28658: Potential directory-traversal via uploaded files
10+
================================================================
11+
12+
``MultiPartParser`` allowed directory-traversal via uploaded files with
13+
suitably crafted file names.
14+
15+
Built-in upload handlers were not affected by this vulnerability.

‎docs/releases/3.0.14.txt‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
===========================
2+
Django 3.0.14 release notes
3+
===========================
4+
5+
*April 6, 2021*
6+
7+
Django 3.0.14 fixes a security issue with severity "low" in 3.0.13.
8+
9+
CVE-2021-28658: Potential directory-traversal via uploaded files
10+
================================================================
11+
12+
``MultiPartParser`` allowed directory-traversal via uploaded files with
13+
suitably crafted file names.
14+
15+
Built-in upload handlers were not affected by this vulnerability.

‎docs/releases/3.1.8.txt‎

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

5-
*Expected April 5, 2021*
5+
*April 6, 2021*
66

7-
Django 3.1.8 fixes several bugs in 3.1.7.
7+
Django 3.1.8 fixes a security issue with severity "low" and a bug in 3.1.7.
8+
9+
CVE-2021-28658: Potential directory-traversal via uploaded files
10+
================================================================
11+
12+
``MultiPartParser`` allowed directory-traversal via uploaded files with
13+
suitably crafted file names.
14+
15+
Built-in upload handlers were not affected by this vulnerability.
816

917
Bugfixes
1018
========

‎docs/releases/index.txt‎

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

50+
3.0.14
5051
3.0.13
5152
3.0.12
5253
3.0.11
@@ -67,6 +68,7 @@ versions of the documentation contain the release notes for any later releases.
6768
.. toctree::
6869
:maxdepth: 1
6970

71+
2.2.20
7072
2.2.19
7173
2.2.18
7274
2.2.17

‎tests/file_uploads/tests.py‎

Lines changed: 68 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,22 @@
2323
MEDIA_ROOT = sys_tempfile.mkdtemp()
2424
UPLOAD_TO = os.path.join(MEDIA_ROOT, 'test_upload')
2525

26+
CANDIDATE_TRAVERSAL_FILE_NAMES = [
27+
'/tmp/hax0rd.txt', # Absolute path, *nix-style.
28+
'C:\\Windows\\hax0rd.txt', # Absolute path, win-style.
29+
'C:/Windows/hax0rd.txt', # Absolute path, broken-style.
30+
'\\tmp\\hax0rd.txt', # Absolute path, broken in a different way.
31+
'/tmp\\hax0rd.txt', # Absolute path, broken by mixing.
32+
'subdir/hax0rd.txt', # Descendant path, *nix-style.
33+
'subdir\\hax0rd.txt', # Descendant path, win-style.
34+
'sub/dir\\hax0rd.txt', # Descendant path, mixed.
35+
'../../hax0rd.txt', # Relative path, *nix-style.
36+
'..\\..\\hax0rd.txt', # Relative path, win-style.
37+
'../..\\hax0rd.txt', # Relative path, mixed.
38+
'../hax0rd.txt', # HTML entities.
39+
'../hax0rd.txt', # HTML entities.
40+
]
41+
2642

2743
@override_settings(MEDIA_ROOT=MEDIA_ROOT, ROOT_URLCONF='file_uploads.urls', MIDDLEWARE=[])
2844
class FileUploadTests(TestCase):
@@ -251,22 +267,8 @@ def test_dangerous_file_names(self):
251267
# a malicious payload with an invalid file name (containing os.sep or
252268
# os.pardir). This similar to what an attacker would need to do when
253269
# trying such an attack.
254-
scary_file_names = [
255-
"/tmp/hax0rd.txt", # Absolute path, *nix-style.
256-
"C:\\Windows\\hax0rd.txt", # Absolute path, win-style.
257-
"C:/Windows/hax0rd.txt", # Absolute path, broken-style.
258-
"\\tmp\\hax0rd.txt", # Absolute path, broken in a different way.
259-
"/tmp\\hax0rd.txt", # Absolute path, broken by mixing.
260-
"subdir/hax0rd.txt", # Descendant path, *nix-style.
261-
"subdir\\hax0rd.txt", # Descendant path, win-style.
262-
"sub/dir\\hax0rd.txt", # Descendant path, mixed.
263-
"../../hax0rd.txt", # Relative path, *nix-style.
264-
"..\\..\\hax0rd.txt", # Relative path, win-style.
265-
"../..\\hax0rd.txt" # Relative path, mixed.
266-
]
267-
268270
payload = client.FakePayload()
269-
for i, name in enumerate(scary_file_names):
271+
for i, name in enumerate(CANDIDATE_TRAVERSAL_FILE_NAMES):
270272
payload.write('\r\n'.join([
271273
'--' + client.BOUNDARY,
272274
'Content-Disposition: form-data; name="file%s"; filename="%s"' % (i, name),
@@ -286,7 +288,7 @@ def test_dangerous_file_names(self):
286288
response = self.client.request(**r)
287289
# The filenames should have been sanitized by the time it got to the view.
288290
received = response.json()
289-
for i, name in enumerate(scary_file_names):
291+
for i, name in enumerate(CANDIDATE_TRAVERSAL_FILE_NAMES):
290292
got = received["file%s" % i]
291293
self.assertEqual(got, "hax0rd.txt")
292294

@@ -597,6 +599,47 @@ def test_filename_case_preservation(self):
597599
# shouldn't differ.
598600
self.assertEqual(os.path.basename(obj.testfile.path), 'MiXeD_cAsE.txt')
599601

602+
def test_filename_traversal_upload(self):
603+
os.makedirs(UPLOAD_TO, exist_ok=True)
604+
self.addCleanup(shutil.rmtree, MEDIA_ROOT)
605+
tests = [
606+
'../test.txt',
607+
'../test.txt',
608+
]
609+
for file_name in tests:
610+
with self.subTest(file_name=file_name):
611+
payload = client.FakePayload()
612+
payload.write(
613+
'\r\n'.join([
614+
'--' + client.BOUNDARY,
615+
'Content-Disposition: form-data; name="my_file"; '
616+
'filename="%s";' % file_name,
617+
'Content-Type: text/plain',
618+
'',
619+
'file contents.\r\n',
620+
'\r\n--' + client.BOUNDARY + '--\r\n',
621+
]),
622+
)
623+
r = {
624+
'CONTENT_LENGTH': len(payload),
625+
'CONTENT_TYPE': client.MULTIPART_CONTENT,
626+
'PATH_INFO': '/upload_traversal/',
627+
'REQUEST_METHOD': 'POST',
628+
'wsgi.input': payload,
629+
}
630+
response = self.client.request(**r)
631+
result = response.json()
632+
self.assertEqual(response.status_code, 200)
633+
self.assertEqual(result['file_name'], 'test.txt')
634+
self.assertIs(
635+
os.path.exists(os.path.join(MEDIA_ROOT, 'test.txt')),
636+
False,
637+
)
638+
self.assertIs(
639+
os.path.exists(os.path.join(UPLOAD_TO, 'test.txt')),
640+
True,
641+
)
642+
600643

601644
@override_settings(MEDIA_ROOT=MEDIA_ROOT)
602645
class DirectoryCreationTests(SimpleTestCase):
@@ -666,6 +709,15 @@ def test_bad_type_content_length(self):
666709
}, StringIO('x'), [], 'utf-8')
667710
self.assertEqual(multipart_parser._content_length, 0)
668711

712+
def test_sanitize_file_name(self):
713+
parser = MultiPartParser({
714+
'CONTENT_TYPE': 'multipart/form-data; boundary=_foo',
715+
'CONTENT_LENGTH': '1'
716+
}, StringIO('x'), [], 'utf-8')
717+
for file_name in CANDIDATE_TRAVERSAL_FILE_NAMES:
718+
with self.subTest(file_name=file_name):
719+
self.assertEqual(parser.sanitize_file_name(file_name), 'hax0rd.txt')
720+
669721
def test_rfc2231_parsing(self):
670722
test_data = (
671723
(b"Content-Type: application/x-stuff; title*=us-ascii'en-us'This%20is%20%2A%2A%2Afun%2A%2A%2A",

‎tests/file_uploads/uploadhandler.py‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
"""
22
Upload handlers to test the upload API.
33
"""
4+
import os
5+
from tempfile import NamedTemporaryFile
46

57
from django.core.files.uploadhandler import (
68
FileUploadHandler, StopUpload, TemporaryFileUploadHandler,
@@ -43,3 +45,32 @@ class ErroringUploadHandler(FileUploadHandler):
4345
"""A handler that raises an exception."""
4446
def receive_data_chunk(self, raw_data, start):
4547
raise CustomUploadError("Oops!")
48+
49+
50+
class TraversalUploadHandler(FileUploadHandler):
51+
"""A handler with potential directory-traversal vulnerability."""
52+
def __init__(self, request=None):
53+
from .views import UPLOAD_TO
54+
55+
super().__init__(request)
56+
self.upload_dir = UPLOAD_TO
57+
58+
def file_complete(self, file_size):
59+
self.file.seek(0)
60+
self.file.size = file_size
61+
with open(os.path.join(self.upload_dir, self.file_name), 'wb') as fp:
62+
fp.write(self.file.read())
63+
return self.file
64+
65+
def new_file(
66+
self, field_name, file_name, content_type, content_length, charset=None,
67+
content_type_extra=None,
68+
):
69+
super().new_file(
70+
file_name, file_name, content_length, content_length, charset,
71+
content_type_extra,
72+
)
73+
self.file = NamedTemporaryFile(suffix='.upload', dir=self.upload_dir)
74+
75+
def receive_data_chunk(self, raw_data, start):
76+
self.file.write(raw_data)

‎tests/file_uploads/urls.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
urlpatterns = [
66
path('upload/', views.file_upload_view),
7+
path('upload_traversal/', views.file_upload_traversal_view),
78
path('verify/', views.file_upload_view_verify),
89
path('unicode_name/', views.file_upload_unicode_name),
910
path('echo/', views.file_upload_echo),

‎tests/file_uploads/views.py‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
from .tests import UNICODE_FILENAME, UPLOAD_TO
1010
from .uploadhandler import (
1111
ErroringUploadHandler, QuotaUploadHandler, StopUploadTemporaryFileHandler,
12+
TraversalUploadHandler,
1213
)
1314

1415

@@ -162,3 +163,11 @@ def file_upload_fd_closing(request, access):
162163
if access == 't':
163164
request.FILES # Trigger file parsing.
164165
return HttpResponse()
166+
167+
168+
def file_upload_traversal_view(request):
169+
request.upload_handlers.insert(0, TraversalUploadHandler())
170+
request.FILES # Trigger file parsing.
171+
return JsonResponse(
172+
{'file_name': request.upload_handlers[0].file_name},
173+
)

0 commit comments

Comments
 (0)