From be8237c7cce24b06aabde0b97afce98ddabbe3b6 Mon Sep 17 00:00:00 2001 From: Nick Pope Date: Tue, 16 Feb 2021 10:14:17 +0000 Subject: [PATCH] [3.2.x] Fixed CVE-2021-23336 -- Fixed web cache poisoning via django.utils.http.parse_qsl(). --- django/http/request.py | 5 ++- django/utils/http.py | 16 +++++--- docs/releases/2.2.19.txt | 16 ++++++++ docs/releases/3.0.13.txt | 16 ++++++++ docs/releases/3.1.7.txt | 13 ++++++- docs/releases/index.txt | 2 + tests/handlers/test_exception.py | 2 +- tests/requests/test_data_upload_settings.py | 8 ++-- tests/utils_tests/test_http.py | 43 ++++++++++++++------- 9 files changed, 94 insertions(+), 27 deletions(-) create mode 100644 docs/releases/2.2.19.txt create mode 100644 docs/releases/3.0.13.txt diff --git a/django/http/request.py b/django/http/request.py index 2488bf9ccd..195341ec4b 100644 --- a/django/http/request.py +++ b/django/http/request.py @@ -29,7 +29,10 @@ from .multipartparser import parse_header # detect whether the max_num_fields argument is available as this security fix # was backported to Python 3.6.8 and 3.7.2, and may also have been applied by # downstream package maintainers to other versions in their repositories. -if not func_supports_parameter(parse_qsl, 'max_num_fields'): +if ( + not func_supports_parameter(parse_qsl, 'max_num_fields') or + not func_supports_parameter(parse_qsl, 'separator') +): from django.utils.http import parse_qsl diff --git a/django/utils/http.py b/django/utils/http.py index 810d7970ba..61db5e6028 100644 --- a/django/utils/http.py +++ b/django/utils/http.py @@ -415,13 +415,13 @@ def _url_has_allowed_host_and_scheme(url, allowed_hosts, require_https=False): # TODO: Remove when dropping support for PY37. def parse_qsl( qs, keep_blank_values=False, strict_parsing=False, encoding='utf-8', - errors='replace', max_num_fields=None, + errors='replace', max_num_fields=None, separator='&', ): """ Return a list of key/value tuples parsed from query string. - Backport of urllib.parse.parse_qsl() from Python 3.8. - Copyright (C) 2020 Python Software Foundation (see LICENSE.python). + Backport of urllib.parse.parse_qsl() from Python 3.8.8. + Copyright (C) 2021 Python Software Foundation (see LICENSE.python). ---- @@ -447,19 +447,25 @@ def parse_qsl( max_num_fields: int. If set, then throws a ValueError if there are more than n fields read by parse_qsl(). + separator: str. The symbol to use for separating the query arguments. + Defaults to &. + Returns a list, as G-d intended. """ qs, _coerce_result = _coerce_args(qs) + if not separator or not isinstance(separator, (str, bytes)): + raise ValueError('Separator must be of type string or bytes.') + # If max_num_fields is defined then check that the number of fields is less # than max_num_fields. This prevents a memory exhaustion DOS attack via # post bodies with many fields. if max_num_fields is not None: - num_fields = 1 + qs.count('&') + qs.count(';') + num_fields = 1 + qs.count(separator) if max_num_fields < num_fields: raise ValueError('Max number of fields exceeded') - pairs = [s2 for s1 in qs.split('&') for s2 in s1.split(';')] + pairs = [s1 for s1 in qs.split(separator)] r = [] for name_value in pairs: if not name_value and not strict_parsing: diff --git a/docs/releases/2.2.19.txt b/docs/releases/2.2.19.txt new file mode 100644 index 0000000000..feaffd996c --- /dev/null +++ b/docs/releases/2.2.19.txt @@ -0,0 +1,16 @@ +=========================== +Django 2.2.19 release notes +=========================== + +*February 19, 2021* + +Django 2.2.19 fixes a security issue in 2.2.18. + +CVE-2021-23336: Web cache poisoning via ``django.utils.http.limited_parse_qsl()`` +================================================================================= + +Django contains a copy of :func:`urllib.parse.parse_qsl` which was added to +backport some security fixes. A further security fix has been issued recently +such that ``parse_qsl()`` no longer allows using ``;`` as a query parameter +separator by default. Django now includes this fix. See :bpo:`42967` for +further details. diff --git a/docs/releases/3.0.13.txt b/docs/releases/3.0.13.txt new file mode 100644 index 0000000000..c78b8a04fd --- /dev/null +++ b/docs/releases/3.0.13.txt @@ -0,0 +1,16 @@ +=========================== +Django 3.0.13 release notes +=========================== + +*February 19, 2021* + +Django 3.0.13 fixes a security issue in 3.0.12. + +CVE-2021-23336: Web cache poisoning via ``django.utils.http.limited_parse_qsl()`` +================================================================================= + +Django contains a copy of :func:`urllib.parse.parse_qsl` which was added to +backport some security fixes. A further security fix has been issued recently +such that ``parse_qsl()`` no longer allows using ``;`` as a query parameter +separator by default. Django now includes this fix. See :bpo:`42967` for +further details. diff --git a/docs/releases/3.1.7.txt b/docs/releases/3.1.7.txt index 1ef16b76c7..e58fc23e80 100644 --- a/docs/releases/3.1.7.txt +++ b/docs/releases/3.1.7.txt @@ -2,9 +2,18 @@ Django 3.1.7 release notes ========================== -*Expected March 1, 2021* +*February 19, 2021* -Django 3.1.7 fixes several bugs in 3.1.6. +Django 3.1.7 fixes a security issue and a bug in 3.1.6. + +CVE-2021-23336: Web cache poisoning via ``django.utils.http.limited_parse_qsl()`` +================================================================================= + +Django contains a copy of :func:`urllib.parse.parse_qsl` which was added to +backport some security fixes. A further security fix has been issued recently +such that ``parse_qsl()`` no longer allows using ``;`` as a query parameter +separator by default. Django now includes this fix. See :bpo:`42967` for +further details. Bugfixes ======== diff --git a/docs/releases/index.txt b/docs/releases/index.txt index 0751de5d1c..f5f3f277f0 100644 --- a/docs/releases/index.txt +++ b/docs/releases/index.txt @@ -46,6 +46,7 @@ versions of the documentation contain the release notes for any later releases. .. toctree:: :maxdepth: 1 + 3.0.13 3.0.12 3.0.11 3.0.10 @@ -65,6 +66,7 @@ versions of the documentation contain the release notes for any later releases. .. toctree:: :maxdepth: 1 + 2.2.19 2.2.18 2.2.17 2.2.16 diff --git a/tests/handlers/test_exception.py b/tests/handlers/test_exception.py index 7afd4acc6b..0c1e763990 100644 --- a/tests/handlers/test_exception.py +++ b/tests/handlers/test_exception.py @@ -6,7 +6,7 @@ from django.test.client import FakePayload class ExceptionHandlerTests(SimpleTestCase): def get_suspicious_environ(self): - payload = FakePayload('a=1&a=2;a=3\r\n') + payload = FakePayload('a=1&a=2&a=3\r\n') return { 'REQUEST_METHOD': 'POST', 'CONTENT_TYPE': 'application/x-www-form-urlencoded', diff --git a/tests/requests/test_data_upload_settings.py b/tests/requests/test_data_upload_settings.py index f60f1850ea..44897cc9fa 100644 --- a/tests/requests/test_data_upload_settings.py +++ b/tests/requests/test_data_upload_settings.py @@ -11,7 +11,7 @@ TOO_MUCH_DATA_MSG = 'Request body exceeded settings.DATA_UPLOAD_MAX_MEMORY_SIZE. class DataUploadMaxMemorySizeFormPostTests(SimpleTestCase): def setUp(self): - payload = FakePayload('a=1&a=2;a=3\r\n') + payload = FakePayload('a=1&a=2&a=3\r\n') self.request = WSGIRequest({ 'REQUEST_METHOD': 'POST', 'CONTENT_TYPE': 'application/x-www-form-urlencoded', @@ -117,7 +117,7 @@ class DataUploadMaxNumberOfFieldsGet(SimpleTestCase): request = WSGIRequest({ 'REQUEST_METHOD': 'GET', 'wsgi.input': BytesIO(b''), - 'QUERY_STRING': 'a=1&a=2;a=3', + 'QUERY_STRING': 'a=1&a=2&a=3', }) request.GET['a'] @@ -126,7 +126,7 @@ class DataUploadMaxNumberOfFieldsGet(SimpleTestCase): request = WSGIRequest({ 'REQUEST_METHOD': 'GET', 'wsgi.input': BytesIO(b''), - 'QUERY_STRING': 'a=1&a=2;a=3', + 'QUERY_STRING': 'a=1&a=2&a=3', }) request.GET['a'] @@ -168,7 +168,7 @@ class DataUploadMaxNumberOfFieldsMultipartPost(SimpleTestCase): class DataUploadMaxNumberOfFieldsFormPost(SimpleTestCase): def setUp(self): - payload = FakePayload("\r\n".join(['a=1&a=2;a=3', ''])) + payload = FakePayload("\r\n".join(['a=1&a=2&a=3', ''])) self.request = WSGIRequest({ 'REQUEST_METHOD': 'POST', 'CONTENT_TYPE': 'application/x-www-form-urlencoded', diff --git a/tests/utils_tests/test_http.py b/tests/utils_tests/test_http.py index 1966386e77..bd44ee307a 100644 --- a/tests/utils_tests/test_http.py +++ b/tests/utils_tests/test_http.py @@ -363,8 +363,8 @@ class EscapeLeadingSlashesTests(unittest.TestCase): # TODO: Remove when dropping support for PY37. Backport of unit tests for -# urllib.parse.parse_qsl() from Python 3.8. Copyright (C) 2020 Python Software -# Foundation (see LICENSE.python). +# urllib.parse.parse_qsl() from Python 3.8.8. Copyright (C) 2021 Python +# Software Foundation (see LICENSE.python). class ParseQSLBackportTests(unittest.TestCase): def test_parse_qsl(self): tests = [ @@ -388,16 +388,10 @@ class ParseQSLBackportTests(unittest.TestCase): (b'&a=b', [(b'a', b'b')]), (b'a=a+b&b=b+c', [(b'a', b'a b'), (b'b', b'b c')]), (b'a=1&a=2', [(b'a', b'1'), (b'a', b'2')]), - (';', []), - (';;', []), - (';a=b', [('a', 'b')]), - ('a=a+b;b=b+c', [('a', 'a b'), ('b', 'b c')]), - ('a=1;a=2', [('a', '1'), ('a', '2')]), - (b';', []), - (b';;', []), - (b';a=b', [(b'a', b'b')]), - (b'a=a+b;b=b+c', [(b'a', b'a b'), (b'b', b'b c')]), - (b'a=1;a=2', [(b'a', b'1'), (b'a', b'2')]), + (';a=b', [(';a', 'b')]), + ('a=a+b;b=b+c', [('a', 'a b;b=b c')]), + (b';a=b', [(b';a', b'b')]), + (b'a=a+b;b=b+c', [(b'a', b'a b;b=b c')]), ] for original, expected in tests: with self.subTest(original): @@ -422,6 +416,27 @@ class ParseQSLBackportTests(unittest.TestCase): def test_parse_qsl_max_num_fields(self): with self.assertRaises(ValueError): parse_qsl('&'.join(['a=a'] * 11), max_num_fields=10) - with self.assertRaises(ValueError): - parse_qsl(';'.join(['a=a'] * 11), max_num_fields=10) parse_qsl('&'.join(['a=a'] * 10), max_num_fields=10) + + def test_parse_qsl_separator(self): + tests = [ + (';', []), + (';;', []), + ('=;a', []), + (';a=b', [('a', 'b')]), + ('a=a+b;b=b+c', [('a', 'a b'), ('b', 'b c')]), + ('a=1;a=2', [('a', '1'), ('a', '2')]), + (b';', []), + (b';;', []), + (b';a=b', [(b'a', b'b')]), + (b'a=a+b;b=b+c', [(b'a', b'a b'), (b'b', b'b c')]), + (b'a=1;a=2', [(b'a', b'1'), (b'a', b'2')]), + ] + for original, expected in tests: + with self.subTest(original): + result = parse_qsl(original, separator=';') + self.assertEqual(result, expected, 'Error parsing %r' % original) + + def test_parse_qsl_bad_separator(self): + with self.assertRaisesRegex(ValueError, 'Separator must be of type string or bytes.'): + parse_qsl('a=b0c=d', separator=0)