[3.2.x] Fixed CVE-2021-23336 -- Fixed web cache poisoning via django.utils.http.parse_qsl().

ngnpope · carltongibson · commit be8237c7cce2 · 2021-02-19T09:15:09.000+01:00
diff --git a/django/http/request.py b/django/http/request.py
@@ -29,7 +29,10 @@
 # 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -6,7 +6,7 @@
 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
@@ -11,7 +11,7 @@
 
 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 @@ def test_get_max_fields_exceeded(self):
                 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 @@ def test_get_max_fields_not_exceeded(self):
             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 @@ def test_no_limit(self):
 
 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
@@ -363,8 +363,8 @@ def test(self):
 
 
 # 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 @@ def test_parse_qsl(self):
             (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 @@ def test_parse_qsl_encoding(self):
     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)
