Skip to content

Commit e7fba62

Browse files
felixxmfelixxm
committed
[3.0.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 232d5f6 commit e7fba62

File tree

8 files changed

+152
-21
lines changed

8 files changed

+152
-21
lines changed

‎django/http/multipartparser.py‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import cgi
1010
import collections
1111
import html
12+
import os
1213
from urllib.parse import unquote
1314

1415
from django.conf import settings
@@ -209,7 +210,7 @@ def parse(self):
209210
file_name = disposition.get('filename')
210211
if file_name:
211212
file_name = force_str(file_name, encoding, errors='replace')
212-
file_name = self.IE_sanitize(html.unescape(file_name))
213+
file_name = self.sanitize_file_name(file_name)
213214
if not file_name:
214215
continue
215216

@@ -297,9 +298,13 @@ def handle_file_complete(self, old_field_name, counters):
297298
self._files.appendlist(force_str(old_field_name, self._encoding, errors='replace'), file_obj)
298299
break
299300

300-
def IE_sanitize(self, filename):
301-
"""Cleanup filename from Internet Explorer full paths."""
302-
return filename and filename[filename.rfind("\\") + 1:].strip()
301+
def sanitize_file_name(self, file_name):
302+
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)
306+
307+
IE_sanitize = sanitize_file_name
303308

304309
def _close_files(self):
305310
# 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/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.0.14
2829
3.0.13
2930
3.0.12
3031
3.0.11
@@ -45,6 +46,7 @@ versions of the documentation contain the release notes for any later releases.
4546
.. toctree::
4647
:maxdepth: 1
4748

49+
2.2.20
4850
2.2.19
4951
2.2.18
5052
2.2.17

‎tests/file_uploads/tests.py‎

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

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

2642
@override_settings(MEDIA_ROOT=MEDIA_ROOT, ROOT_URLCONF='file_uploads.urls', MIDDLEWARE=[])
2743
class FileUploadTests(TestCase):
@@ -204,22 +220,8 @@ def test_dangerous_file_names(self):
204220
# a malicious payload with an invalid file name (containing os.sep or
205221
# os.pardir). This similar to what an attacker would need to do when
206222
# trying such an attack.
207-
scary_file_names = [
208-
"/tmp/hax0rd.txt", # Absolute path, *nix-style.
209-
"C:\\Windows\\hax0rd.txt", # Absolute path, win-style.
210-
"C:/Windows/hax0rd.txt", # Absolute path, broken-style.
211-
"\\tmp\\hax0rd.txt", # Absolute path, broken in a different way.
212-
"/tmp\\hax0rd.txt", # Absolute path, broken by mixing.
213-
"subdir/hax0rd.txt", # Descendant path, *nix-style.
214-
"subdir\\hax0rd.txt", # Descendant path, win-style.
215-
"sub/dir\\hax0rd.txt", # Descendant path, mixed.
216-
"../../hax0rd.txt", # Relative path, *nix-style.
217-
"..\\..\\hax0rd.txt", # Relative path, win-style.
218-
"../..\\hax0rd.txt" # Relative path, mixed.
219-
]
220-
221223
payload = client.FakePayload()
222-
for i, name in enumerate(scary_file_names):
224+
for i, name in enumerate(CANDIDATE_TRAVERSAL_FILE_NAMES):
223225
payload.write('\r\n'.join([
224226
'--' + client.BOUNDARY,
225227
'Content-Disposition: form-data; name="file%s"; filename="%s"' % (i, name),
@@ -239,7 +241,7 @@ def test_dangerous_file_names(self):
239241
response = self.client.request(**r)
240242
# The filenames should have been sanitized by the time it got to the view.
241243
received = response.json()
242-
for i, name in enumerate(scary_file_names):
244+
for i, name in enumerate(CANDIDATE_TRAVERSAL_FILE_NAMES):
243245
got = received["file%s" % i]
244246
self.assertEqual(got, "hax0rd.txt")
245247

@@ -517,6 +519,47 @@ def test_filename_case_preservation(self):
517519
# shouldn't differ.
518520
self.assertEqual(os.path.basename(obj.testfile.path), 'MiXeD_cAsE.txt')
519521

522+
def test_filename_traversal_upload(self):
523+
os.makedirs(UPLOAD_TO, exist_ok=True)
524+
self.addCleanup(shutil.rmtree, MEDIA_ROOT)
525+
tests = [
526+
'../test.txt',
527+
'../test.txt',
528+
]
529+
for file_name in tests:
530+
with self.subTest(file_name=file_name):
531+
payload = client.FakePayload()
532+
payload.write(
533+
'\r\n'.join([
534+
'--' + client.BOUNDARY,
535+
'Content-Disposition: form-data; name="my_file"; '
536+
'filename="%s";' % file_name,
537+
'Content-Type: text/plain',
538+
'',
539+
'file contents.\r\n',
540+
'\r\n--' + client.BOUNDARY + '--\r\n',
541+
]),
542+
)
543+
r = {
544+
'CONTENT_LENGTH': len(payload),
545+
'CONTENT_TYPE': client.MULTIPART_CONTENT,
546+
'PATH_INFO': '/upload_traversal/',
547+
'REQUEST_METHOD': 'POST',
548+
'wsgi.input': payload,
549+
}
550+
response = self.client.request(**r)
551+
result = response.json()
552+
self.assertEqual(response.status_code, 200)
553+
self.assertEqual(result['file_name'], 'test.txt')
554+
self.assertIs(
555+
os.path.exists(os.path.join(MEDIA_ROOT, 'test.txt')),
556+
False,
557+
)
558+
self.assertIs(
559+
os.path.exists(os.path.join(UPLOAD_TO, 'test.txt')),
560+
True,
561+
)
562+
520563

521564
@override_settings(MEDIA_ROOT=MEDIA_ROOT)
522565
class DirectoryCreationTests(SimpleTestCase):
@@ -586,6 +629,15 @@ def test_bad_type_content_length(self):
586629
}, StringIO('x'), [], 'utf-8')
587630
self.assertEqual(multipart_parser._content_length, 0)
588631

632+
def test_sanitize_file_name(self):
633+
parser = MultiPartParser({
634+
'CONTENT_TYPE': 'multipart/form-data; boundary=_foo',
635+
'CONTENT_LENGTH': '1'
636+
}, StringIO('x'), [], 'utf-8')
637+
for file_name in CANDIDATE_TRAVERSAL_FILE_NAMES:
638+
with self.subTest(file_name=file_name):
639+
self.assertEqual(parser.sanitize_file_name(file_name), 'hax0rd.txt')
640+
589641
def test_rfc2231_parsing(self):
590642
test_data = (
591643
(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 FileUploadHandler, StopUpload
68

@@ -35,3 +37,32 @@ class ErroringUploadHandler(FileUploadHandler):
3537
"""A handler that raises an exception."""
3638
def receive_data_chunk(self, raw_data, start):
3739
raise CustomUploadError("Oops!")
40+
41+
42+
class TraversalUploadHandler(FileUploadHandler):
43+
"""A handler with potential directory-traversal vulnerability."""
44+
def __init__(self, request=None):
45+
from .views import UPLOAD_TO
46+
47+
super().__init__(request)
48+
self.upload_dir = UPLOAD_TO
49+
50+
def file_complete(self, file_size):
51+
self.file.seek(0)
52+
self.file.size = file_size
53+
with open(os.path.join(self.upload_dir, self.file_name), 'wb') as fp:
54+
fp.write(self.file.read())
55+
return self.file
56+
57+
def new_file(
58+
self, field_name, file_name, content_type, content_length, charset=None,
59+
content_type_extra=None,
60+
):
61+
super().new_file(
62+
file_name, file_name, content_length, content_length, charset,
63+
content_type_extra,
64+
)
65+
self.file = NamedTemporaryFile(suffix='.upload', dir=self.upload_dir)
66+
67+
def receive_data_chunk(self, raw_data, start):
68+
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: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@
66

77
from .models import FileModel
88
from .tests import UNICODE_FILENAME, UPLOAD_TO
9-
from .uploadhandler import ErroringUploadHandler, QuotaUploadHandler
9+
from .uploadhandler import (
10+
ErroringUploadHandler, QuotaUploadHandler, TraversalUploadHandler,
11+
)
1012

1113

1214
def file_upload_view(request):
@@ -141,3 +143,11 @@ def file_upload_fd_closing(request, access):
141143
if access == 't':
142144
request.FILES # Trigger file parsing.
143145
return HttpResponse()
146+
147+
148+
def file_upload_traversal_view(request):
149+
request.upload_handlers.insert(0, TraversalUploadHandler())
150+
request.FILES # Trigger file parsing.
151+
return JsonResponse(
152+
{'file_name': request.upload_handlers[0].file_name},
153+
)

0 commit comments

Comments
 (0)