File CVE-2023-24580-DOS_file_upload.patch of Package python-Django.27745
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', '']))
