diff --git a/django/conf/global_settings.py b/django/conf/global_settings.py index 4cca441560..5b15d9617d 100644 --- a/django/conf/global_settings.py +++ b/django/conf/global_settings.py @@ -313,6 +313,10 @@ DATA_UPLOAD_MAX_MEMORY_SIZE = 2621440 # i.e. 2.5 MB # 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 646644f3e0..2a2288ff4d 100644 --- a/django/core/exceptions.py +++ b/django/core/exceptions.py @@ -67,6 +67,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 a0b1ba678a..a63291f3b9 100644 --- a/django/core/handlers/exception.py +++ b/django/core/handlers/exception.py @@ -12,6 +12,7 @@ from django.core.exceptions import ( RequestDataTooBig, SuspiciousOperation, TooManyFieldsSent, + TooManyFilesSent, ) from django.http import Http404 from django.http.multipartparser import MultiPartParserError @@ -110,7 +111,7 @@ def response_for_exception(request, exc): exception=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() diff --git a/django/http/multipartparser.py b/django/http/multipartparser.py index 8f0d85c3d8..5ab63455ef 100644 --- a/django/http/multipartparser.py +++ b/django/http/multipartparser.py @@ -14,6 +14,7 @@ from django.core.exceptions import ( RequestDataTooBig, SuspiciousMultipartForm, TooManyFieldsSent, + TooManyFilesSent, ) from django.core.files.uploadhandler import SkipFile, StopFutureHandlers, StopUpload from django.utils.datastructures import MultiValueDict @@ -39,6 +40,7 @@ class InputStreamExhausted(Exception): RAW = "raw" FILE = "file" FIELD = "field" +FIELD_TYPES = frozenset([FIELD, RAW]) class MultiPartParser: @@ -111,6 +113,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. @@ -156,6 +174,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 # Whether a file upload is finished. @@ -171,6 +191,20 @@ class MultiPartParser: 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() @@ -183,17 +217,6 @@ class MultiPartParser: 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 = ( @@ -228,6 +251,16 @@ class MultiPartParser: 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: @@ -305,8 +338,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: diff --git a/django/http/request.py b/django/http/request.py index d451147bc1..2ef9dfd649 100644 --- a/django/http/request.py +++ b/django/http/request.py @@ -13,7 +13,11 @@ from django.core.exceptions import ( 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, @@ -382,7 +386,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 diff --git a/docs/ref/exceptions.txt b/docs/ref/exceptions.txt index b588d0ee81..54a31115f0 100644 --- a/docs/ref/exceptions.txt +++ b/docs/ref/exceptions.txt @@ -95,12 +95,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 ` 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 d54b9be8cb..50e6241cff 100644 --- a/docs/ref/settings.txt +++ b/docs/ref/settings.txt @@ -1062,6 +1062,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`` @@ -3685,6 +3707,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/docs/releases/3.2.18.txt b/docs/releases/3.2.18.txt index 431d04c989..46c0feb51e 100644 --- a/docs/releases/3.2.18.txt +++ b/docs/releases/3.2.18.txt @@ -6,4 +6,12 @@ Django 3.2.18 release notes Django 3.2.18 fixes a security issue with severity "moderate" in 3.2.17. -... +CVE-2023-24580: Potential denial-of-service vulnerability in file uploads +========================================================================= + +Passing certain inputs to multipart forms could result in too many open files +or memory exhaustion, and provided a potential vector for a denial-of-service +attack. + +The number of files parts parsed is now limited via the new +:setting:`DATA_UPLOAD_MAX_NUMBER_FILES` setting. diff --git a/docs/releases/4.0.10.txt b/docs/releases/4.0.10.txt index b01f8c5b1b..4d076ab40e 100644 --- a/docs/releases/4.0.10.txt +++ b/docs/releases/4.0.10.txt @@ -6,4 +6,12 @@ Django 4.0.10 release notes Django 4.0.10 fixes a security issue with severity "moderate" in 4.0.9. -... +CVE-2023-24580: Potential denial-of-service vulnerability in file uploads +========================================================================= + +Passing certain inputs to multipart forms could result in too many open files +or memory exhaustion, and provided a potential vector for a denial-of-service +attack. + +The number of files parts parsed is now limited via the new +:setting:`DATA_UPLOAD_MAX_NUMBER_FILES` setting. diff --git a/docs/releases/4.1.7.txt b/docs/releases/4.1.7.txt index e74d43c0e5..bcc6858d3e 100644 --- a/docs/releases/4.1.7.txt +++ b/docs/releases/4.1.7.txt @@ -4,10 +4,18 @@ Django 4.1.7 release notes *February 14, 2023* -Django 4.1.7 fixes a security issue with severity "moderate" and several bugs -in 4.1.6. +Django 4.1.7 fixes a security issue with severity "moderate" and a bug in +4.1.6. -... +CVE-2023-24580: Potential denial-of-service vulnerability in file uploads +========================================================================= + +Passing certain inputs to multipart forms could result in too many open files +or memory exhaustion, and provided a potential vector for a denial-of-service +attack. + +The number of files parts parsed is now limited via the new +:setting:`DATA_UPLOAD_MAX_NUMBER_FILES` setting. Bugfixes ======== diff --git a/tests/handlers/test_exception.py b/tests/handlers/test_exception.py index 3a483be784..878fff7cc0 100644 --- a/tests/handlers/test_exception.py +++ b/tests/handlers/test_exception.py @@ -1,6 +1,11 @@ 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): @@ -24,3 +29,27 @@ class ExceptionHandlerTests(SimpleTestCase): 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_tests/test_data_upload_settings.py b/tests/requests_tests/test_data_upload_settings.py index 0199296293..e89af0a39b 100644 --- a/tests/requests_tests/test_data_upload_settings.py +++ b/tests/requests_tests/test_data_upload_settings.py @@ -1,6 +1,10 @@ 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 @@ -8,6 +12,9 @@ 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." @@ -191,6 +198,52 @@ class DataUploadMaxNumberOfFieldsMultipartPost(SimpleTestCase): 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", ""]))