packages/python-django
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity
python-django/CVE-2023-24580.patch
starlet-dx 7e01621ce0 Fix CVE-2023-24580 
(cherry picked from commit d400ae309e430361099c4decc57bc1a7a47fb8ee)
2023-02-25 17:58:47 +08:00

402 lines
16 KiB
Diff
Raw Permalink Blame History

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 ++++++++
docs/releases/3.2.18.txt | 10 +++-
tests/handlers/test_exception.py | 28 +++++++++-
tests/requests/test_data_upload_settings.py | 51 ++++++++++++++++-
10 files changed, 184 insertions(+), 18 deletions(-)
diff --git a/django/conf/global_settings.py b/django/conf/global_settings.py
index cf9fae496e3a..4a27887a8f04 100644
--- a/django/conf/global_settings.py
+++ b/django/conf/global_settings.py
@@ -303,6 +303,10 @@ def gettext_noop(s):
# 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).
diff --git a/django/core/exceptions.py b/django/core/exceptions.py
index 673d004d5756..83161a58cd66 100644
--- a/django/core/exceptions.py
+++ b/django/core/exceptions.py
@@ -58,6 +58,15 @@ class TooManyFieldsSent(SuspiciousOperation):
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
diff --git a/django/core/handlers/exception.py b/django/core/handlers/exception.py
index 3005a5eccb11..2ecc2a0fd697 100644
--- a/django/core/handlers/exception.py
+++ b/django/core/handlers/exception.py
@@ -9,7 +9,7 @@
from django.core import signals
from django.core.exceptions import (
BadRequest, PermissionDenied, RequestDataTooBig, SuspiciousOperation,
- TooManyFieldsSent,
+ TooManyFieldsSent, TooManyFilesSent,
)
from django.http import Http404
from django.http.multipartparser import MultiPartParserError
@@ -88,7 +88,7 @@ def response_for_exception(request, exc):
exc_info=sys.exc_info(),
)
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()
diff --git a/django/http/multipartparser.py b/django/http/multipartparser.py
index 35a54f4ca12e..d8a304d4babe 100644
--- a/django/http/multipartparser.py
+++ b/django/http/multipartparser.py
@@ -14,6 +14,7 @@
from django.conf import settings
from django.core.exceptions import (
RequestDataTooBig, SuspiciousMultipartForm, TooManyFieldsSent,
+ TooManyFilesSent,
)
from django.core.files.uploadhandler import (
SkipFile, StopFutureHandlers, StopUpload,
@@ -38,6 +39,7 @@ class InputStreamExhausted(Exception):
RAW = "raw"
FILE = "file"
FIELD = "field"
+FIELD_TYPES = frozenset([FIELD, RAW])
class MultiPartParser:
@@ -102,6 +104,22 @@ def __init__(self, META, input_data, upload_handlers, encoding=None):
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.
@@ -147,6 +165,8 @@ def parse(self):
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
# Whether a file upload is finished.
@@ -162,6 +182,20 @@ def parse(self):
old_field_name = None
uploaded_file = True
+ 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()
@@ -174,15 +208,6 @@ def parse(self):
field_name = force_str(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
@@ -208,6 +233,16 @@ def parse(self):
self._post.appendlist(field_name, force_str(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:
@@ -276,8 +311,13 @@ def parse(self):
# 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:
diff --git a/django/http/request.py b/django/http/request.py
index 195341ec4b69..b6cd7a372f14 100644
--- a/django/http/request.py
+++ b/django/http/request.py
@@ -12,7 +12,9 @@
DisallowedHost, ImproperlyConfigured, RequestDataTooBig, TooManyFieldsSent,
)
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 (
CaseInsensitiveMapping, ImmutableList, MultiValueDict,
)
@@ -360,7 +362,7 @@ def _load_post_and_files(self):
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
diff --git a/docs/ref/exceptions.txt b/docs/ref/exceptions.txt
index 2f5aa64b9d9d..7d34025cd65c 100644
--- a/docs/ref/exceptions.txt
+++ b/docs/ref/exceptions.txt
@@ -84,12 +84,17 @@ Django core exception classes are defined in ``django.core.exceptions``.
* ``SuspiciousMultipartForm``
* ``SuspiciousSession``
* ``TooManyFieldsSent``
+ * ``TooManyFilesSent``
If a ``SuspiciousOperation`` exception reaches the ASGI/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``
--------------------
diff --git a/docs/ref/settings.txt b/docs/ref/settings.txt
index 9bfadbc89bd2..9173009c94d5 100644
--- a/docs/ref/settings.txt
+++ b/docs/ref/settings.txt
@@ -1063,6 +1063,28 @@ 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:: 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``
@@ -3671,6 +3693,7 @@ HTTP
----
* :setting:`DATA_UPLOAD_MAX_MEMORY_SIZE`
* :setting:`DATA_UPLOAD_MAX_NUMBER_FIELDS`
+* :setting:`DATA_UPLOAD_MAX_NUMBER_FILES`
* :setting:`DEFAULT_CHARSET`
* :setting:`DISALLOWED_USER_AGENTS`
* :setting:`FORCE_SCRIPT_NAME`
diff --git a/tests/handlers/test_exception.py b/tests/handlers/test_exception.py
index 0c1e76399045..7de2edaeea34 100644
--- 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 @@ def test_data_upload_max_memory_size_exceeded(self):
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)
diff --git a/tests/requests/test_data_upload_settings.py b/tests/requests/test_data_upload_settings.py
index 44897cc9fa97..ded778b42286 100644
--- 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 @@ def test_no_limit(self):
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', '']))
Reference in New Issue View Git Blame Copy Permalink