File CVE-2023-24580-DOS_file_upload.patch of Package python-Django

Overview Repositories Revisions Requests Users Attributes Meta

File CVE-2023-24580-DOS_file_upload.patch of Package python-Django (Revision 6ed5ad8e2d490bb6593e857c9a32164b)

Currently displaying revision 6ed5ad8e2d490bb6593e857c9a32164b , Show latest

From a665ed5179f5bbd3db95ce67286d0192eff041d8 Mon Sep 17 00:00:00 2001
From: Markus Holtermann <info@markusholtermann.eu>
Date: Tue, 13 Dec 2022 10:27:39 +0100
Subject: [PATCH] [3.2.x] Fixed CVE-2023-24580 -- Prevented DoS with too many
 uploaded files.

Thanks to Jakob Ackermann for the report.
---
 django/conf/global_settings.py              |    4 +
 django/core/exceptions.py                   |    9 ++++
 django/core/handlers/exception.py           |    4 -
 django/http/multipartparser.py              |   62 +++++++++++++++++++++++-----
 django/http/request.py                      |    6 +-
 docs/ref/exceptions.txt                     |    5 ++
 docs/ref/settings.txt                       |   23 ++++++++++
 tests/handlers/test_exception.py            |   28 ++++++++++++
 tests/requests/test_data_upload_settings.py |   51 ++++++++++++++++++++++-
 9 files changed, 175 insertions(+), 17 deletions(-)

--- a/django/conf/global_settings.py
+++ b/django/conf/global_settings.py
@@ -303,6 +303,10 @@ DATA_UPLOAD_MAX_MEMORY_SIZE = 2621440  #
 # SuspiciousOperation (TooManyFieldsSent) is raised.
 DATA_UPLOAD_MAX_NUMBER_FIELDS = 1000
 
+# Maximum number of files encoded in a multipart upload that will be read
+# before a SuspiciousOperation (TooManyFilesSent) is raised.
+DATA_UPLOAD_MAX_NUMBER_FILES = 100
+
 # Directory in which upload streamed files will be temporarily saved. A value of
 # `None` will make Django use the operating system's default temporary directory
 # (i.e. "/tmp" on *nix systems).
--- a/django/core/exceptions.py
+++ b/django/core/exceptions.py
@@ -55,6 +55,15 @@ class TooManyFieldsSent(SuspiciousOperat
     pass
 
 
+class TooManyFilesSent(SuspiciousOperation):
+    """
+    The number of fields in a GET or POST request exceeded
+    settings.DATA_UPLOAD_MAX_NUMBER_FILES.
+    """
+
+    pass
+
+
 class RequestDataTooBig(SuspiciousOperation):
     """
     The size of the request (excluding any file uploads) exceeded
--- a/django/core/handlers/exception.py
+++ b/django/core/handlers/exception.py
@@ -6,7 +6,7 @@ from django.conf import settings
 from django.core import signals
 from django.core.exceptions import (
     PermissionDenied, RequestDataTooBig, SuspiciousOperation,
-    TooManyFieldsSent,
+    TooManyFieldsSent, TooManyFilesSent,
 )
 from django.http import Http404
 from django.http.multipartparser import MultiPartParserError
@@ -61,7 +61,7 @@ def response_for_exception(request, exc)
         response = get_exception_response(request, get_resolver(get_urlconf()), 400, exc)
 
     elif isinstance(exc, SuspiciousOperation):
-        if isinstance(exc, (RequestDataTooBig, TooManyFieldsSent)):
+        if isinstance(exc, (RequestDataTooBig, TooManyFieldsSent, TooManyFilesSent)):
             # POST data can't be accessed again, otherwise the original
             # exception would be raised.
             request._mark_post_parse_error()
--- a/django/http/multipartparser.py
+++ b/django/http/multipartparser.py
@@ -12,6 +12,7 @@ from urllib.parse import unquote
 from django.conf import settings
 from django.core.exceptions import (
     RequestDataTooBig, SuspiciousMultipartForm, TooManyFieldsSent,
+    TooManyFilesSent,
 )
 from django.core.files.uploadhandler import (
     SkipFile, StopFutureHandlers, StopUpload,
@@ -37,6 +38,7 @@ class InputStreamExhausted(Exception):
 RAW = "raw"
 FILE = "file"
 FIELD = "field"
+FIELD_TYPES = frozenset([FIELD, RAW])
 
 
 class MultiPartParser:
@@ -98,6 +100,22 @@ class MultiPartParser:
         self._upload_handlers = upload_handlers
 
     def parse(self):
+        # Call the actual parse routine and close all open files in case of
+        # errors. This is needed because if exceptions are thrown the
+        # MultiPartParser will not be garbage collected immediately and
+        # resources would be kept alive. This is only needed for errors because
+        # the Request object closes all uploaded files at the end of the
+        # request.
+        try:
+            return self._parse()
+        except Exception:
+            if hasattr(self, "_files"):
+                for _, files in self._files.lists():
+                    for fileobj in files:
+                        fileobj.close()
+            raise
+
+    def _parse(self):
         """
         Parse the POST data and break it into a FILES MultiValueDict and a POST
         MultiValueDict.
@@ -143,6 +161,8 @@ class MultiPartParser:
         num_bytes_read = 0
         # To count the number of keys in the request.
         num_post_keys = 0
+        # To count the number of files in the request.
+        num_files = 0
         # To limit the amount of data read from the request.
         read_size = None
 
@@ -155,6 +175,20 @@ class MultiPartParser:
                     self.handle_file_complete(old_field_name, counters)
                     old_field_name = None
 
+                if (
+                    item_type in FIELD_TYPES and
+                    settings.DATA_UPLOAD_MAX_NUMBER_FIELDS is not None
+                ):
+                    # Avoid storing more than DATA_UPLOAD_MAX_NUMBER_FIELDS.
+                    num_post_keys += 1
+                    # 2 accounts for empty raw fields before and after the
+                    # last boundary.
+                    if settings.DATA_UPLOAD_MAX_NUMBER_FIELDS + 2 < num_post_keys:
+                        raise TooManyFieldsSent(
+                            "The number of GET/POST parameters exceeded "
+                            "settings.DATA_UPLOAD_MAX_NUMBER_FIELDS."
+                        )
+
                 try:
                     disposition = meta_data['content-disposition'][1]
                     field_name = disposition['name'].strip()
@@ -167,15 +201,6 @@ class MultiPartParser:
                 field_name = force_text(field_name, encoding, errors='replace')
 
                 if item_type == FIELD:
-                    # Avoid storing more than DATA_UPLOAD_MAX_NUMBER_FIELDS.
-                    num_post_keys += 1
-                    if (settings.DATA_UPLOAD_MAX_NUMBER_FIELDS is not None and
-                            settings.DATA_UPLOAD_MAX_NUMBER_FIELDS < num_post_keys):
-                        raise TooManyFieldsSent(
-                            'The number of GET/POST parameters exceeded '
-                            'settings.DATA_UPLOAD_MAX_NUMBER_FIELDS.'
-                        )
-
                     # Avoid reading more than DATA_UPLOAD_MAX_MEMORY_SIZE.
                     if settings.DATA_UPLOAD_MAX_MEMORY_SIZE is not None:
                         read_size = settings.DATA_UPLOAD_MAX_MEMORY_SIZE - num_bytes_read
@@ -201,6 +226,16 @@ class MultiPartParser:
 
                     self._post.appendlist(field_name, force_text(data, encoding, errors='replace'))
                 elif item_type == FILE:
+                    # Avoid storing more than DATA_UPLOAD_MAX_NUMBER_FILES.
+                    num_files += 1
+                    if (
+                        settings.DATA_UPLOAD_MAX_NUMBER_FILES is not None and
+                        num_files > settings.DATA_UPLOAD_MAX_NUMBER_FILES
+                    ):
+                        raise TooManyFilesSent(
+                            "The number of files exceeded "
+                            "settings.DATA_UPLOAD_MAX_NUMBER_FILES."
+                        )
                     # This is a file, use the handler...
                     file_name = disposition.get('filename')
                     if file_name:
@@ -266,8 +301,13 @@ class MultiPartParser:
                         # Handle file upload completions on next iteration.
                         old_field_name = field_name
                 else:
-                    # If this is neither a FIELD or a FILE, just exhaust the stream.
-                    exhaust(stream)
+                    # If this is neither a FIELD nor a FILE, exhaust the field
+                    # stream. Note: There could be an error here at some point,
+                    # but there will be at least two RAW types (before and
+                    # after the other boundaries). This branch is usually not
+                    # reached at all, because a missing content-disposition
+                    # header will skip the whole boundary.
+                    exhaust(field_stream)
         except StopUpload as e:
             self._close_files()
             if not e.connection_reset:
--- a/django/http/request.py
+++ b/django/http/request.py
@@ -11,7 +11,9 @@ from django.core.exceptions import (
     DisallowedHost, ImproperlyConfigured, RequestDataTooBig,
 )
 from django.core.files import uploadhandler
-from django.http.multipartparser import MultiPartParser, MultiPartParserError
+from django.http.multipartparser import (
+    MultiPartParser, MultiPartParserError, TooManyFilesSent,
+)
 from django.utils.datastructures import ImmutableList, MultiValueDict
 from django.utils.deprecation import RemovedInDjango30Warning
 from django.utils.encoding import escape_uri_path, force_bytes, iri_to_uri
@@ -288,7 +290,7 @@ class HttpRequest:
                 data = self
             try:
                 self._post, self._files = self.parse_file_upload(self.META, data)
-            except MultiPartParserError:
+            except (MultiPartParserError, TooManyFilesSent):
                 # An error occurred while parsing POST data. Since when
                 # formatting the error the request handler might access
                 # self.POST, set self._post and self._file to prevent
--- a/docs/ref/exceptions.txt
+++ b/docs/ref/exceptions.txt
@@ -88,12 +88,17 @@ Django core exception classes are define
     * ``SuspiciousMultipartForm``
     * ``SuspiciousSession``
     * ``TooManyFieldsSent``
+    * ``TooManyFilesSent``
 
     If a ``SuspiciousOperation`` exception reaches the WSGI handler level it is
     logged at the ``Error`` level and results in
     a :class:`~django.http.HttpResponseBadRequest`. See the :doc:`logging
     documentation </topics/logging/>` for more information.
 
+.. versionchanged:: 3.2.18
+
+    ``SuspiciousOperation`` is raised when too many files are submitted.
+
 ``PermissionDenied``
 --------------------
 
--- a/docs/ref/settings.txt
+++ b/docs/ref/settings.txt
@@ -1004,6 +1004,28 @@ could be used as a denial-of-service att
 servers don't typically perform deep request inspection, it's not possible to
 perform a similar check at that level.
 
+.. setting:: DATA_UPLOAD_MAX_NUMBER_FILES
+
+``DATA_UPLOAD_MAX_NUMBER_FILES``
+--------------------------------
+
+.. versionadded:: 3.2.18
+
+Default: ``100``
+
+The maximum number of files that may be received via POST in a
+``multipart/form-data`` encoded request before a
+:exc:`~django.core.exceptions.SuspiciousOperation` (``TooManyFiles``) is
+raised. You can set this to ``None`` to disable the check. Applications that
+are expected to receive an unusually large number of file fields should tune
+this setting.
+
+The number of accepted files is correlated to the amount of time and memory
+needed to process the request. Large requests could be used as a
+denial-of-service attack vector if left unchecked. Since web servers don't
+typically perform deep request inspection, it's not possible to perform a
+similar check at that level.
+
 .. setting:: DATABASE_ROUTERS
 
 ``DATABASE_ROUTERS``
@@ -3430,6 +3452,7 @@ HTTP
 ----
 * :setting:`DATA_UPLOAD_MAX_MEMORY_SIZE`
 * :setting:`DATA_UPLOAD_MAX_NUMBER_FIELDS`
+* :setting:`DATA_UPLOAD_MAX_NUMBER_FILES`
 * :setting:`DEFAULT_CHARSET`
 * :setting:`DEFAULT_CONTENT_TYPE`
 * :setting:`DISALLOWED_USER_AGENTS`
--- a/tests/handlers/test_exception.py
+++ b/tests/handlers/test_exception.py
@@ -1,6 +1,8 @@
 from django.core.handlers.wsgi import WSGIHandler
 from django.test import SimpleTestCase, override_settings
-from django.test.client import FakePayload
+from django.test.client import (
+    BOUNDARY, MULTIPART_CONTENT, FakePayload, encode_multipart,
+)
 
 
 class ExceptionHandlerTests(SimpleTestCase):
@@ -25,3 +27,27 @@ class ExceptionHandlerTests(SimpleTestCa
     def test_data_upload_max_number_fields_exceeded(self):
         response = WSGIHandler()(self.get_suspicious_environ(), lambda *a, **k: None)
         self.assertEqual(response.status_code, 400)
+
+    @override_settings(DATA_UPLOAD_MAX_NUMBER_FILES=2)
+    def test_data_upload_max_number_files_exceeded(self):
+        payload = FakePayload(
+            encode_multipart(
+                BOUNDARY,
+                {
+                    "a.txt": "Hello World!",
+                    "b.txt": "Hello Django!",
+                    "c.txt": "Hello Python!",
+                },
+            )
+        )
+        environ = {
+            "REQUEST_METHOD": "POST",
+            "CONTENT_TYPE": MULTIPART_CONTENT,
+            "CONTENT_LENGTH": len(payload),
+            "wsgi.input": payload,
+            "SERVER_NAME": "test",
+            "SERVER_PORT": "8000",
+        }
+
+        response = WSGIHandler()(environ, lambda *a, **k: None)
+        self.assertEqual(response.status_code, 400)
--- a/tests/requests/test_data_upload_settings.py
+++ b/tests/requests/test_data_upload_settings.py
@@ -1,11 +1,14 @@
 from io import BytesIO
 
-from django.core.exceptions import RequestDataTooBig, TooManyFieldsSent
+from django.core.exceptions import (
+    RequestDataTooBig, TooManyFieldsSent, TooManyFilesSent,
+)
 from django.core.handlers.wsgi import WSGIRequest
 from django.test import SimpleTestCase
 from django.test.client import FakePayload
 
 TOO_MANY_FIELDS_MSG = 'The number of GET/POST parameters exceeded settings.DATA_UPLOAD_MAX_NUMBER_FIELDS.'
+TOO_MANY_FILES_MSG = 'The number of files exceeded settings.DATA_UPLOAD_MAX_NUMBER_FILES.'
 TOO_MUCH_DATA_MSG = 'Request body exceeded settings.DATA_UPLOAD_MAX_MEMORY_SIZE.'
 
 
@@ -166,6 +169,52 @@ class DataUploadMaxNumberOfFieldsMultipa
             self.request._load_post_and_files()
 
 
+class DataUploadMaxNumberOfFilesMultipartPost(SimpleTestCase):
+    def setUp(self):
+        payload = FakePayload(
+            "\r\n".join(
+                [
+                    "--boundary",
+                    (
+                        'Content-Disposition: form-data; name="name1"; '
+                        'filename="name1.txt"'
+                    ),
+                    "",
+                    "value1",
+                    "--boundary",
+                    (
+                        'Content-Disposition: form-data; name="name2"; '
+                        'filename="name2.txt"'
+                    ),
+                    "",
+                    "value2",
+                    "--boundary--",
+                ]
+            )
+        )
+        self.request = WSGIRequest(
+            {
+                "REQUEST_METHOD": "POST",
+                "CONTENT_TYPE": "multipart/form-data; boundary=boundary",
+                "CONTENT_LENGTH": len(payload),
+                "wsgi.input": payload,
+            }
+        )
+
+    def test_number_exceeded(self):
+        with self.settings(DATA_UPLOAD_MAX_NUMBER_FILES=1):
+            with self.assertRaisesMessage(TooManyFilesSent, TOO_MANY_FILES_MSG):
+                self.request._load_post_and_files()
+
+    def test_number_not_exceeded(self):
+        with self.settings(DATA_UPLOAD_MAX_NUMBER_FILES=2):
+            self.request._load_post_and_files()
+
+    def test_no_limit(self):
+        with self.settings(DATA_UPLOAD_MAX_NUMBER_FILES=None):
+            self.request._load_post_and_files()
+
+
 class DataUploadMaxNumberOfFieldsFormPost(SimpleTestCase):
     def setUp(self):
         payload = FakePayload("\r\n".join(['a=1&a=2;a=3', '']))

Places

File CVE-2023-24580-DOS_file_upload.patch of Package python-Django (Revision 6ed5ad8e2d490bb6593e857c9a32164b)

Places