Fixed CVE-2021-28658 -- Fixed potential directory-traversal via uploa…

…ded files.

Thanks Claude Paroz for the initial patch.
Thanks Dennis Brinkrolf for the report.

django/http/multipartparser.py

@@ -212,9 +212,8 @@ def parse(self):
                     # 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 @@ def handle_file_complete(self, old_field_name, counters):
                 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.

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.

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.

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
 ========

docs/releases/index.txt

@@ -54,6 +54,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
@@ -74,6 +75,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

tests/file_uploads/tests.py

@@ -23,6 +23,22 @@
 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 @@ def test_dangerous_file_names(self):
         # 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 @@ def test_dangerous_file_names(self):
         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 @@ def test_filename_case_preservation(self):
         # 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 @@ def test_bad_type_content_length(self):
         }, 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",

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)

tests/file_uploads/urls.py

@@ -4,6 +4,7 @@
 
 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),

tests/file_uploads/views.py

@@ -9,6 +9,7 @@
 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},
+    )