From 11bb365c7be4e40707550837f01ba77913ae4659 Mon Sep 17 00:00:00 2001 From: alan Date: Thu, 13 Jul 2023 14:15:15 -0700 Subject: [PATCH] Apply all patches up to CVE-2023-36053 --- django/conf/global_settings.py | 4 ++ django/core/exceptions.py | 9 +++ django/core/handlers/exception.py | 4 +- django/core/validators.py | 11 +++- django/db/backends/sqlite3/schema.py | 2 + django/forms/fields.py | 3 + django/http/multipartparser.py | 62 +++++++++++++++---- django/http/request.py | 6 +- docs/ref/exceptions.txt | 5 ++ docs/ref/settings.txt | 23 +++++++ .../field_tests/test_emailfield.py | 5 +- tests/forms_tests/tests/test_forms.py | 19 ++++-- tests/handlers/test_exception.py | 28 ++++++++- tests/inspectdb/tests.py | 2 +- tests/requests/test_data_upload_settings.py | 51 ++++++++++++++- tests/requests/tests.py | 4 +- tests/validators/tests.py | 6 ++ 17 files changed, 215 insertions(+), 29 deletions(-) diff --git a/django/conf/global_settings.py b/django/conf/global_settings.py index 148c7a0203..e421151c4b 100644 --- a/django/conf/global_settings.py +++ b/django/conf/global_settings.py @@ -304,6 +304,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 47c5359c75..150772a55c 100644 --- a/django/core/exceptions.py +++ b/django/core/exceptions.py @@ -61,6 +61,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 48324372b4..a0d7b9e173 100644 --- a/django/core/handlers/exception.py +++ b/django/core/handlers/exception.py @@ -9,7 +9,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 @@ -67,7 +67,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() diff --git a/django/core/validators.py b/django/core/validators.py index ea18685fdb..079168a558 100644 --- a/django/core/validators.py +++ b/django/core/validators.py @@ -106,6 +106,8 @@ class URLValidator(RegexValidator): r'\Z', re.IGNORECASE) message = _('Enter a valid URL.') schemes = ['http', 'https', 'ftp', 'ftps'] + unsafe_chars = frozenset('\t\r\n') + max_length = 2048 def __init__(self, schemes=None, **kwargs): super(URLValidator, self).__init__(**kwargs) @@ -114,6 +116,10 @@ class URLValidator(RegexValidator): def __call__(self, value): value = force_text(value) + if not isinstance(value, str) or len(value) > self.max_length: + raise ValidationError(self.message, code=self.code, params={'value': value}) + if self.unsafe_chars.intersection(value): + raise ValidationError(self.message, code=self.code) # Check first if the scheme is valid scheme = value.split('://')[0].lower() if scheme not in self.schemes: @@ -194,8 +200,9 @@ class EmailValidator(object): def __call__(self, value): value = force_text(value) - - if not value or '@' not in value: + # The maximum length of an email is 320 characters per RFC 3696 + # section 3. + if not value or '@' not in value or len(value) > 320: raise ValidationError(self.message, code=self.code) user_part, domain_part = value.rsplit('@', 1) diff --git a/django/db/backends/sqlite3/schema.py b/django/db/backends/sqlite3/schema.py index 36adb22584..f8264022f4 100644 --- a/django/db/backends/sqlite3/schema.py +++ b/django/db/backends/sqlite3/schema.py @@ -24,10 +24,12 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): c.execute('PRAGMA foreign_keys') self._initial_pragma_fk = c.fetchone()[0] c.execute('PRAGMA foreign_keys = 0') + self.connection.cursor().execute('PRAGMA legacy_alter_table = ON') return super(DatabaseSchemaEditor, self).__enter__() def __exit__(self, exc_type, exc_value, traceback): super(DatabaseSchemaEditor, self).__exit__(exc_type, exc_value, traceback) + self.connection.cursor().execute('PRAGMA legacy_alter_table = OFF') with self.connection.cursor() as c: # Restore initial FK setting - PRAGMA values can't be parametrized c.execute('PRAGMA foreign_keys = %s' % int(self._initial_pragma_fk)) diff --git a/django/forms/fields.py b/django/forms/fields.py index 734907644a..c6f5c30268 100644 --- a/django/forms/fields.py +++ b/django/forms/fields.py @@ -538,6 +538,9 @@ class EmailField(CharField): default_validators = [validators.validate_email] def __init__(self, *args, **kwargs): + # The default maximum length of an email is 320 characters per RFC 3696 + # section 3. + kwargs.setdefault("max_length", 320) super(EmailField, self).__init__(*args, strip=True, **kwargs) diff --git a/django/http/multipartparser.py b/django/http/multipartparser.py index d9e7e4313f..c7dc99e366 100644 --- a/django/http/multipartparser.py +++ b/django/http/multipartparser.py @@ -14,6 +14,7 @@ import sys from django.conf import settings from django.core.exceptions import ( RequestDataTooBig, SuspiciousMultipartForm, TooManyFieldsSent, + TooManyFilesSent, ) from django.core.files.uploadhandler import ( SkipFile, StopFutureHandlers, StopUpload, @@ -41,6 +42,7 @@ class InputStreamExhausted(Exception): RAW = "raw" FILE = "file" FIELD = "field" +FIELD_TYPES = frozenset([FIELD, RAW]) _BASE64_DECODE_ERROR = TypeError if six.PY2 else binascii.Error @@ -104,6 +106,22 @@ class MultiPartParser(object): 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. @@ -149,6 +167,8 @@ class MultiPartParser(object): 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 @@ -161,6 +181,20 @@ class MultiPartParser(object): 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() @@ -173,15 +207,6 @@ class MultiPartParser(object): 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 @@ -207,6 +232,16 @@ class MultiPartParser(object): 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: @@ -273,8 +308,13 @@ class MultiPartParser(object): # 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 b573cdb180..6989cf79db 100644 --- a/django/http/request.py +++ b/django/http/request.py @@ -12,7 +12,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 import six from django.utils.datastructures import ImmutableList, MultiValueDict from django.utils.encoding import ( @@ -298,7 +300,7 @@ class HttpRequest(object): 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 6ce52977e8..9cca8a5a9f 100644 --- a/docs/ref/exceptions.txt +++ b/docs/ref/exceptions.txt @@ -88,12 +88,17 @@ Django core exception classes are defined in ``django.core.exceptions``. * ``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 ` 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 6b84d34f77..e26f4542c0 100644 --- a/docs/ref/settings.txt +++ b/docs/ref/settings.txt @@ -961,6 +961,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`` @@ -3423,6 +3445,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` diff --git a/tests/forms_tests/field_tests/test_emailfield.py b/tests/forms_tests/field_tests/test_emailfield.py index 906a6cf5ff..16d460022d 100644 --- a/tests/forms_tests/field_tests/test_emailfield.py +++ b/tests/forms_tests/field_tests/test_emailfield.py @@ -11,7 +11,10 @@ class EmailFieldTest(FormFieldAssertionsMixin, SimpleTestCase): def test_emailfield_1(self): f = EmailField() - self.assertWidgetRendersTo(f, '') + self.assertEqual(f.max_length, 320) + self.assertWidgetRendersTo( + f, '' + ) with self.assertRaisesMessage(ValidationError, "'This field is required.'"): f.clean('') with self.assertRaisesMessage(ValidationError, "'This field is required.'"): diff --git a/tests/forms_tests/tests/test_forms.py b/tests/forms_tests/tests/test_forms.py index ffad6f60a6..5234aa47a3 100644 --- a/tests/forms_tests/tests/test_forms.py +++ b/tests/forms_tests/tests/test_forms.py @@ -421,11 +421,18 @@ class FormsTestCase(SimpleTestCase): get_spam = BooleanField() f = SignupForm(auto_id=False) - self.assertHTMLEqual(str(f['email']), '') + self.assertHTMLEqual( + str(f["email"]), + '', + ) self.assertHTMLEqual(str(f['get_spam']), '') f = SignupForm({'email': 'test@example.com', 'get_spam': True}, auto_id=False) - self.assertHTMLEqual(str(f['email']), '') + self.assertHTMLEqual( + str(f["email"]), + '", + ) self.assertHTMLEqual( str(f['get_spam']), '', @@ -2771,7 +2778,7 @@ Good luck picking a username that doesn't already exist.

-
    • +
    • """ ) @@ -2787,7 +2794,7 @@ Good luck picking a username that doesn't already exist.

    -

    +

    """ @@ -2806,7 +2813,7 @@ Good luck picking a username that doesn't already exist.

    - + """ @@ -3405,7 +3412,7 @@ Good luck picking a username that doesn't already exist.

    f = CommentForm(data, auto_id=False, error_class=DivErrorList) self.assertHTMLEqual(f.as_p(), """

    Name:

    Enter a valid email address.
    -

    Email:

    +

    Email:

    This field is required.

    Comment:

    """) diff --git a/tests/handlers/test_exception.py b/tests/handlers/test_exception.py index 7afd4acc6b..91ed71069b 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 @@ 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/inspectdb/tests.py b/tests/inspectdb/tests.py index 316dad4a6f..9784ab6ca4 100644 --- a/tests/inspectdb/tests.py +++ b/tests/inspectdb/tests.py @@ -258,7 +258,7 @@ class InspectDBTestCase(TestCase): } call_command('inspectdb', 'inspectdb_columntypes', stdout=out) output = out.getvalue() - self.assertIn("text_field = myfields.TextField()", output) + self.assertIn("text_field = models.TextField()", output) self.assertIn("big_int_field = models.BigIntegerField()", output) finally: connection.introspection.data_types_reverse = orig_data_types_reverse diff --git a/tests/requests/test_data_upload_settings.py b/tests/requests/test_data_upload_settings.py index f60f1850ea..5a5e81aa54 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 @@ 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', ''])) diff --git a/tests/requests/tests.py b/tests/requests/tests.py index 27dc439e5f..407e4b89f1 100644 --- a/tests/requests/tests.py +++ b/tests/requests/tests.py @@ -243,12 +243,12 @@ class RequestsTests(SimpleTestCase): def test_far_expiration(self): "Cookie will expire when an distant expiration time is provided" response = HttpResponse() - response.set_cookie('datetime', expires=datetime(2028, 1, 1, 4, 5, 6)) + response.set_cookie('datetime', expires=datetime(2038, 1, 1, 4, 5, 6)) datetime_cookie = response.cookies['datetime'] self.assertIn( datetime_cookie['expires'], # assertIn accounts for slight time dependency (#23450) - ('Sat, 01-Jan-2028 04:05:06 GMT', 'Sat, 01-Jan-2028 04:05:07 GMT') + ('Fri, 01-Jan-2038 04:05:06 GMT', 'Fri, 01-Jan-2038 04:05:07 GMT') ) def test_max_age_expiration(self): diff --git a/tests/validators/tests.py b/tests/validators/tests.py index 4ef8a524b1..634cd8f5b7 100644 --- a/tests/validators/tests.py +++ b/tests/validators/tests.py @@ -63,6 +63,7 @@ TEST_DATA = [ (validate_email, 'example@atm.%s' % ('a' * 64), ValidationError), (validate_email, 'example@%s.atm.%s' % ('b' * 64, 'a' * 63), ValidationError), + (validate_email, "example@%scom" % (("a" * 63 + ".") * 100), ValidationError), (validate_email, None, ValidationError), (validate_email, '', ValidationError), (validate_email, 'abc', ValidationError), @@ -223,6 +224,11 @@ TEST_DATA = [ (URLValidator(EXTENDED_SCHEMES), 'git+ssh://git@github.com/example/hg-git.git', None), (URLValidator(EXTENDED_SCHEMES), 'git://-invalid.com', ValidationError), + ( + URLValidator(), + "http://example." + ("a" * 63 + ".") * 1000 + "com", + ValidationError, + ), # Trailing newlines not accepted (URLValidator(), 'http://www.djangoproject.com/\n', ValidationError), (URLValidator(), 'http://[::ffff:192.9.5.5]\n', ValidationError),