[4.0.x] Fixed CVE-2021-45116 -- Fixed potential information disclosure in dictsort template filter.

apollo13 · adamchainz · carltongibson · commit 2a8ec7f546d6 · 2022-01-04T10:10:14.000+01:00
Thanks to Dennis Brinkrolf for the report.

Co-authored-by: Adam Johnson &lt;me@adamj.eu&gt;
diff --git a/django/template/defaultfilters.py b/django/template/defaultfilters.py
@@ -22,7 +22,7 @@
 from django.utils.timesince import timesince, timeuntil
 from django.utils.translation import gettext, ngettext
 
-from .base import Variable, VariableDoesNotExist
+from .base import VARIABLE_ATTRIBUTE_SEPARATOR
 from .library import Library
 
 register = Library()
@@ -503,7 +503,7 @@ def striptags(value):
 def _property_resolver(arg):
     """
     When arg is convertible to float, behave like operator.itemgetter(arg)
-    Otherwise, behave like Variable(arg).resolve
+    Otherwise, chain __getitem__() and getattr().
 
     >>> _property_resolver(1)('abc')
     'b'
@@ -521,7 +521,19 @@ def _property_resolver(arg):
     try:
         float(arg)
     except ValueError:
-        return Variable(arg).resolve
+        if VARIABLE_ATTRIBUTE_SEPARATOR + '_' in arg or arg[0] == '_':
+            raise AttributeError('Access to private variables is forbidden.')
+        parts = arg.split(VARIABLE_ATTRIBUTE_SEPARATOR)
+
+        def resolve(value):
+            for part in parts:
+                try:
+                    value = value[part]
+                except (AttributeError, IndexError, KeyError, TypeError, ValueError):
+                    value = getattr(value, part)
+            return value
+
+        return resolve
     else:
         return itemgetter(arg)
 
@@ -534,7 +546,7 @@ def dictsort(value, arg):
     """
     try:
         return sorted(value, key=_property_resolver(arg))
-    except (TypeError, VariableDoesNotExist):
+    except (AttributeError, TypeError):
         return ''
 
 
@@ -546,7 +558,7 @@ def dictsortreversed(value, arg):
     """
     try:
         return sorted(value, key=_property_resolver(arg), reverse=True)
-    except (TypeError, VariableDoesNotExist):
+    except (AttributeError, TypeError):
         return ''
 
 
diff --git a/docs/ref/templates/builtins.txt b/docs/ref/templates/builtins.txt
@@ -1577,6 +1577,13 @@ produce empty output::
 
     {{ values|dictsort:"0" }}
 
+Ordering by elements at specified index is not supported on dictionaries.
+
+.. versionchanged:: 2.2.26
+
+    In older versions, ordering elements at specified index was supported on
+    dictionaries.
+
 .. templatefilter:: dictsortreversed
 
 ``dictsortreversed``
diff --git a/docs/releases/2.2.26.txt b/docs/releases/2.2.26.txt
@@ -20,3 +20,19 @@ In order to mitigate this issue, relatively long values are now ignored by
 
 This issue has severity "medium" according to the :ref:`Django security policy
 <security-disclosure>`.
+
+CVE-2021-45116: Potential information disclosure in ``dictsort`` template filter
+================================================================================
+
+Due to leveraging the Django Template Language's variable resolution logic, the
+:tfilter:`dictsort` template filter was potentially vulnerable to information
+disclosure or unintended method calls, if passed a suitably crafted key.
+
+In order to avoid this possibility, ``dictsort`` now works with a restricted
+resolution logic, that will not call methods, nor allow indexing on
+dictionaries.
+
+As a reminder, all untrusted user input should be validated before use.
+
+This issue has severity "low" according to the :ref:`Django security policy
+<security-disclosure>`.
diff --git a/docs/releases/3.2.11.txt b/docs/releases/3.2.11.txt
@@ -20,3 +20,19 @@ In order to mitigate this issue, relatively long values are now ignored by
 
 This issue has severity "medium" according to the :ref:`Django security policy
 <security-disclosure>`.
+
+CVE-2021-45116: Potential information disclosure in ``dictsort`` template filter
+================================================================================
+
+Due to leveraging the Django Template Language's variable resolution logic, the
+:tfilter:`dictsort` template filter was potentially vulnerable to information
+disclosure or unintended method calls, if passed a suitably crafted key.
+
+In order to avoid this possibility, ``dictsort`` now works with a restricted
+resolution logic, that will not call methods, nor allow indexing on
+dictionaries.
+
+As a reminder, all untrusted user input should be validated before use.
+
+This issue has severity "low" according to the :ref:`Django security policy
+<security-disclosure>`.
diff --git a/docs/releases/4.0.1.txt b/docs/releases/4.0.1.txt
@@ -21,6 +21,22 @@ In order to mitigate this issue, relatively long values are now ignored by
 This issue has severity "medium" according to the :ref:`Django security policy
 <security-disclosure>`.
 
+CVE-2021-45116: Potential information disclosure in ``dictsort`` template filter
+================================================================================
+
+Due to leveraging the Django Template Language's variable resolution logic, the
+:tfilter:`dictsort` template filter was potentially vulnerable to information
+disclosure or unintended method calls, if passed a suitably crafted key.
+
+In order to avoid this possibility, ``dictsort`` now works with a restricted
+resolution logic, that will not call methods, nor allow indexing on
+dictionaries.
+
+As a reminder, all untrusted user input should be validated before use.
+
+This issue has severity "low" according to the :ref:`Django security policy
+<security-disclosure>`.
+
 Bugfixes
 ========
 
diff --git a/tests/template_tests/filter_tests/test_dictsort.py b/tests/template_tests/filter_tests/test_dictsort.py
@@ -1,9 +1,58 @@
-from django.template.defaultfilters import dictsort
+from django.template.defaultfilters import _property_resolver, dictsort
 from django.test import SimpleTestCase
 
 
+class User:
+    password = 'abc'
+
+    _private = 'private'
+
+    @property
+    def test_property(self):
+        return 'cde'
+
+    def test_method(self):
+        """This is just a test method."""
+
+
 class FunctionTests(SimpleTestCase):
 
+    def test_property_resolver(self):
+        user = User()
+        dict_data = {'a': {
+            'b1': {'c': 'result1'},
+            'b2': user,
+            'b3': {'0': 'result2'},
+            'b4': [0, 1, 2],
+        }}
+        list_data = ['a', 'b', 'c']
+        tests = [
+            ('a.b1.c', dict_data, 'result1'),
+            ('a.b2.password', dict_data, 'abc'),
+            ('a.b2.test_property', dict_data, 'cde'),
+            # The method should not get called.
+            ('a.b2.test_method', dict_data, user.test_method),
+            ('a.b3.0', dict_data, 'result2'),
+            (0, list_data, 'a'),
+        ]
+        for arg, data, expected_value in tests:
+            with self.subTest(arg=arg):
+                self.assertEqual(_property_resolver(arg)(data), expected_value)
+        # Invalid lookups.
+        fail_tests = [
+            ('a.b1.d', dict_data, AttributeError),
+            ('a.b2.password.0', dict_data, AttributeError),
+            ('a.b2._private', dict_data, AttributeError),
+            ('a.b4.0', dict_data, AttributeError),
+            ('a', list_data, AttributeError),
+            ('0', list_data, TypeError),
+            (4, list_data, IndexError),
+        ]
+        for arg, data, expected_exception in fail_tests:
+            with self.subTest(arg=arg):
+                with self.assertRaises(expected_exception):
+                    _property_resolver(arg)(data)
+
     def test_sort(self):
         sorted_dicts = dictsort(
             [{'age': 23, 'name': 'Barbara-Ann'},
@@ -21,7 +70,7 @@ def test_sort(self):
 
     def test_dictsort_complex_sorting_key(self):
         """
-        Since dictsort uses template.Variable under the hood, it can sort
+        Since dictsort uses dict.get()/getattr() under the hood, it can sort
         on keys like 'foo.bar'.
         """
         data = [
@@ -60,3 +109,9 @@ def test_invalid_values(self):
         self.assertEqual(dictsort('Hello!', 'age'), '')
         self.assertEqual(dictsort({'a': 1}, 'age'), '')
         self.assertEqual(dictsort(1, 'age'), '')
+
+    def test_invalid_args(self):
+        """Fail silently if invalid lookups are passed."""
+        self.assertEqual(dictsort([{}], '._private'), '')
+        self.assertEqual(dictsort([{'_private': 'test'}], '_private'), '')
+        self.assertEqual(dictsort([{'nested': {'_private': 'test'}}], 'nested._private'), '')
diff --git a/tests/template_tests/filter_tests/test_dictsortreversed.py b/tests/template_tests/filter_tests/test_dictsortreversed.py
@@ -46,3 +46,9 @@ def test_invalid_values(self):
         self.assertEqual(dictsortreversed('Hello!', 'age'), '')
         self.assertEqual(dictsortreversed({'a': 1}, 'age'), '')
         self.assertEqual(dictsortreversed(1, 'age'), '')
+
+    def test_invalid_args(self):
+        """Fail silently if invalid lookups are passed."""
+        self.assertEqual(dictsortreversed([{}], '._private'), '')
+        self.assertEqual(dictsortreversed([{'_private': 'test'}], '_private'), '')
+        self.assertEqual(dictsortreversed([{'nested': {'_private': 'test'}}], 'nested._private'), '')
