From 2820fd1be5dfccbf1216c3845fad8580502473e1 Mon Sep 17 00:00:00 2001 From: Mariusz Felisiak Date: Tue, 16 Mar 2021 10:19:00 +0100 Subject: [PATCH] [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 d4d800ca1addc4141e03c5440a849bb64d1582cd from main. --- django/http/multipartparser.py | 13 +++-- docs/releases/2.2.20.txt | 15 ++++++ docs/releases/3.0.14.txt | 15 ++++++ docs/releases/3.1.8.txt | 12 ++++- docs/releases/index.txt | 2 + tests/file_uploads/tests.py | 84 +++++++++++++++++++++++------ tests/file_uploads/uploadhandler.py | 31 +++++++++++ tests/file_uploads/urls.py | 1 + tests/file_uploads/views.py | 9 ++++ 9 files changed, 159 insertions(+), 23 deletions(-) create mode 100644 docs/releases/2.2.20.txt create mode 100644 docs/releases/3.0.14.txt diff --git a/django/http/multipartparser.py b/django/http/multipartparser.py index 8078393a668..180a533bb6a 100644 --- a/django/http/multipartparser.py +++ b/django/http/multipartparser.py @@ -212,9 +212,8 @@ class MultiPartParser: # This is a file, use the handler... file_name = disposition.get('filename') if file_name: - file_name = os.path.basename(file_name) file_name = force_str(file_name, encoding, errors='replace') - file_name = self.IE_sanitize(html.unescape(file_name)) + file_name = self.sanitize_file_name(file_name) if not file_name: continue @@ -306,9 +305,13 @@ class MultiPartParser: self._files.appendlist(force_str(old_field_name, self._encoding, errors='replace'), file_obj) break - def IE_sanitize(self, filename): - """Cleanup filename from Internet Explorer full paths.""" - return filename and filename[filename.rfind("\\") + 1:].strip() + def sanitize_file_name(self, file_name): + file_name = html.unescape(file_name) + # Cleanup Windows-style path separators. + file_name = file_name[file_name.rfind('\\') + 1:].strip() + return os.path.basename(file_name) + + IE_sanitize = sanitize_file_name def _close_files(self): # Free up all file handles. diff --git a/docs/releases/2.2.20.txt b/docs/releases/2.2.20.txt new file mode 100644 index 00000000000..a67c5150218 --- /dev/null +++ b/docs/releases/2.2.20.txt @@ -0,0 +1,15 @@ +=========================== +Django 2.2.20 release notes +=========================== + +*April 6, 2021* + +Django 2.2.20 fixes a security issue with severity "low" in 2.2.19. + +CVE-2021-28658: Potential directory-traversal via uploaded files +================================================================ + +``MultiPartParser`` allowed directory-traversal via uploaded files with +suitably crafted file names. + +Built-in upload handlers were not affected by this vulnerability. diff --git a/docs/releases/3.0.14.txt b/docs/releases/3.0.14.txt new file mode 100644 index 00000000000..c3244287452 --- /dev/null +++ b/docs/releases/3.0.14.txt @@ -0,0 +1,15 @@ +=========================== +Django 3.0.14 release notes +=========================== + +*April 6, 2021* + +Django 3.0.14 fixes a security issue with severity "low" in 3.0.13. + +CVE-2021-28658: Potential directory-traversal via uploaded files +================================================================ + +``MultiPartParser`` allowed directory-traversal via uploaded files with +suitably crafted file names. + +Built-in upload handlers were not affected by this vulnerability. diff --git a/docs/releases/3.1.8.txt b/docs/releases/3.1.8.txt index d166a1200cf..cf04b89c366 100644 --- a/docs/releases/3.1.8.txt +++ b/docs/releases/3.1.8.txt @@ -2,9 +2,17 @@ Django 3.1.8 release notes ========================== -*Expected April 5, 2021* +*April 6, 2021* -Django 3.1.8 fixes several bugs in 3.1.7. +Django 3.1.8 fixes a security issue with severity "low" and a bug in 3.1.7. + +CVE-2021-28658: Potential directory-traversal via uploaded files +================================================================ + +``MultiPartParser`` allowed directory-traversal via uploaded files with +suitably crafted file names. + +Built-in upload handlers were not affected by this vulnerability. Bugfixes ======== diff --git a/docs/releases/index.txt b/docs/releases/index.txt index f77e6342cc2..7fa2d0a8e02 100644 --- a/docs/releases/index.txt +++ b/docs/releases/index.txt @@ -47,6 +47,7 @@ versions of the documentation contain the release notes for any later releases. .. toctree:: :maxdepth: 1 + 3.0.14 3.0.13 3.0.12 3.0.11 @@ -67,6 +68,7 @@ versions of the documentation contain the release notes for any later releases. .. toctree:: :maxdepth: 1 + 2.2.20 2.2.19 2.2.18 2.2.17 diff --git a/tests/file_uploads/tests.py b/tests/file_uploads/tests.py index 2a1b2998e53..e8f91e2fa04 100644 --- a/tests/file_uploads/tests.py +++ b/tests/file_uploads/tests.py @@ -23,6 +23,22 @@ UNICODE_FILENAME = 'test-0123456789_中文_Orléans.jpg' MEDIA_ROOT = sys_tempfile.mkdtemp() UPLOAD_TO = os.path.join(MEDIA_ROOT, 'test_upload') +CANDIDATE_TRAVERSAL_FILE_NAMES = [ + '/tmp/hax0rd.txt', # Absolute path, *nix-style. + 'C:\\Windows\\hax0rd.txt', # Absolute path, win-style. + 'C:/Windows/hax0rd.txt', # Absolute path, broken-style. + '\\tmp\\hax0rd.txt', # Absolute path, broken in a different way. + '/tmp\\hax0rd.txt', # Absolute path, broken by mixing. + 'subdir/hax0rd.txt', # Descendant path, *nix-style. + 'subdir\\hax0rd.txt', # Descendant path, win-style. + 'sub/dir\\hax0rd.txt', # Descendant path, mixed. + '../../hax0rd.txt', # Relative path, *nix-style. + '..\\..\\hax0rd.txt', # Relative path, win-style. + '../..\\hax0rd.txt', # Relative path, mixed. + '../hax0rd.txt', # HTML entities. + '../hax0rd.txt', # HTML entities. +] + @override_settings(MEDIA_ROOT=MEDIA_ROOT, ROOT_URLCONF='file_uploads.urls', MIDDLEWARE=[]) class FileUploadTests(TestCase): @@ -251,22 +267,8 @@ class FileUploadTests(TestCase): # a malicious payload with an invalid file name (containing os.sep or # os.pardir). This similar to what an attacker would need to do when # trying such an attack. - scary_file_names = [ - "/tmp/hax0rd.txt", # Absolute path, *nix-style. - "C:\\Windows\\hax0rd.txt", # Absolute path, win-style. - "C:/Windows/hax0rd.txt", # Absolute path, broken-style. - "\\tmp\\hax0rd.txt", # Absolute path, broken in a different way. - "/tmp\\hax0rd.txt", # Absolute path, broken by mixing. - "subdir/hax0rd.txt", # Descendant path, *nix-style. - "subdir\\hax0rd.txt", # Descendant path, win-style. - "sub/dir\\hax0rd.txt", # Descendant path, mixed. - "../../hax0rd.txt", # Relative path, *nix-style. - "..\\..\\hax0rd.txt", # Relative path, win-style. - "../..\\hax0rd.txt" # Relative path, mixed. - ] - payload = client.FakePayload() - for i, name in enumerate(scary_file_names): + for i, name in enumerate(CANDIDATE_TRAVERSAL_FILE_NAMES): payload.write('\r\n'.join([ '--' + client.BOUNDARY, 'Content-Disposition: form-data; name="file%s"; filename="%s"' % (i, name), @@ -286,7 +288,7 @@ class FileUploadTests(TestCase): response = self.client.request(**r) # The filenames should have been sanitized by the time it got to the view. received = response.json() - for i, name in enumerate(scary_file_names): + for i, name in enumerate(CANDIDATE_TRAVERSAL_FILE_NAMES): got = received["file%s" % i] self.assertEqual(got, "hax0rd.txt") @@ -597,6 +599,47 @@ class FileUploadTests(TestCase): # shouldn't differ. self.assertEqual(os.path.basename(obj.testfile.path), 'MiXeD_cAsE.txt') + def test_filename_traversal_upload(self): + os.makedirs(UPLOAD_TO, exist_ok=True) + self.addCleanup(shutil.rmtree, MEDIA_ROOT) + tests = [ + '../test.txt', + '../test.txt', + ] + for file_name in tests: + with self.subTest(file_name=file_name): + payload = client.FakePayload() + payload.write( + '\r\n'.join([ + '--' + client.BOUNDARY, + 'Content-Disposition: form-data; name="my_file"; ' + 'filename="%s";' % file_name, + 'Content-Type: text/plain', + '', + 'file contents.\r\n', + '\r\n--' + client.BOUNDARY + '--\r\n', + ]), + ) + r = { + 'CONTENT_LENGTH': len(payload), + 'CONTENT_TYPE': client.MULTIPART_CONTENT, + 'PATH_INFO': '/upload_traversal/', + 'REQUEST_METHOD': 'POST', + 'wsgi.input': payload, + } + response = self.client.request(**r) + result = response.json() + self.assertEqual(response.status_code, 200) + self.assertEqual(result['file_name'], 'test.txt') + self.assertIs( + os.path.exists(os.path.join(MEDIA_ROOT, 'test.txt')), + False, + ) + self.assertIs( + os.path.exists(os.path.join(UPLOAD_TO, 'test.txt')), + True, + ) + @override_settings(MEDIA_ROOT=MEDIA_ROOT) class DirectoryCreationTests(SimpleTestCase): @@ -666,6 +709,15 @@ class MultiParserTests(SimpleTestCase): }, StringIO('x'), [], 'utf-8') self.assertEqual(multipart_parser._content_length, 0) + def test_sanitize_file_name(self): + parser = MultiPartParser({ + 'CONTENT_TYPE': 'multipart/form-data; boundary=_foo', + 'CONTENT_LENGTH': '1' + }, StringIO('x'), [], 'utf-8') + for file_name in CANDIDATE_TRAVERSAL_FILE_NAMES: + with self.subTest(file_name=file_name): + self.assertEqual(parser.sanitize_file_name(file_name), 'hax0rd.txt') + def test_rfc2231_parsing(self): test_data = ( (b"Content-Type: application/x-stuff; title*=us-ascii'en-us'This%20is%20%2A%2A%2Afun%2A%2A%2A", diff --git a/tests/file_uploads/uploadhandler.py b/tests/file_uploads/uploadhandler.py index 9ad335301f9..eac6de037ca 100644 --- a/tests/file_uploads/uploadhandler.py +++ b/tests/file_uploads/uploadhandler.py @@ -1,6 +1,8 @@ """ Upload handlers to test the upload API. """ +import os +from tempfile import NamedTemporaryFile from django.core.files.uploadhandler import ( FileUploadHandler, StopUpload, TemporaryFileUploadHandler, @@ -43,3 +45,32 @@ class ErroringUploadHandler(FileUploadHandler): """A handler that raises an exception.""" def receive_data_chunk(self, raw_data, start): raise CustomUploadError("Oops!") + + +class TraversalUploadHandler(FileUploadHandler): + """A handler with potential directory-traversal vulnerability.""" + def __init__(self, request=None): + from .views import UPLOAD_TO + + super().__init__(request) + self.upload_dir = UPLOAD_TO + + def file_complete(self, file_size): + self.file.seek(0) + self.file.size = file_size + with open(os.path.join(self.upload_dir, self.file_name), 'wb') as fp: + fp.write(self.file.read()) + return self.file + + def new_file( + self, field_name, file_name, content_type, content_length, charset=None, + content_type_extra=None, + ): + super().new_file( + file_name, file_name, content_length, content_length, charset, + content_type_extra, + ) + self.file = NamedTemporaryFile(suffix='.upload', dir=self.upload_dir) + + def receive_data_chunk(self, raw_data, start): + self.file.write(raw_data) diff --git a/tests/file_uploads/urls.py b/tests/file_uploads/urls.py index d171be2c76e..9df5432403f 100644 --- a/tests/file_uploads/urls.py +++ b/tests/file_uploads/urls.py @@ -4,6 +4,7 @@ from . import views urlpatterns = [ path('upload/', views.file_upload_view), + path('upload_traversal/', views.file_upload_traversal_view), path('verify/', views.file_upload_view_verify), path('unicode_name/', views.file_upload_unicode_name), path('echo/', views.file_upload_echo), diff --git a/tests/file_uploads/views.py b/tests/file_uploads/views.py index d521f001fed..50de6238b42 100644 --- a/tests/file_uploads/views.py +++ b/tests/file_uploads/views.py @@ -9,6 +9,7 @@ from .models import FileModel from .tests import UNICODE_FILENAME, UPLOAD_TO from .uploadhandler import ( ErroringUploadHandler, QuotaUploadHandler, StopUploadTemporaryFileHandler, + TraversalUploadHandler, ) @@ -162,3 +163,11 @@ def file_upload_fd_closing(request, access): if access == 't': request.FILES # Trigger file parsing. return HttpResponse() + + +def file_upload_traversal_view(request): + request.upload_handlers.insert(0, TraversalUploadHandler()) + request.FILES # Trigger file parsing. + return JsonResponse( + {'file_name': request.upload_handlers[0].file_name}, + )