From 761f449e0daf3de06b0132bd4d6dfcdeef578e26 Mon Sep 17 00:00:00 2001 From: Florian Apolloner Date: Mon, 27 Dec 2021 14:53:18 +0100 Subject: [PATCH] Fixed CVE-2021-45116 -- Fixed potential information disclosure in dictsort template filter. Thanks to Dennis Brinkrolf for the report. Co-authored-by: Adam Johnson --- django/template/defaultfilters.py | 22 +++++-- docs/ref/templates/builtins.txt | 7 +++ docs/releases/2.2.26.txt | 16 +++++ docs/releases/3.2.11.txt | 16 +++++ docs/releases/4.0.1.txt | 16 +++++ .../filter_tests/test_dictsort.py | 59 ++++++++++++++++++- .../filter_tests/test_dictsortreversed.py | 6 ++ 7 files changed, 135 insertions(+), 7 deletions(-) diff --git a/django/template/defaultfilters.py b/django/template/defaultfilters.py index 13070b303bd..0813cc9dad6 100644 --- a/django/template/defaultfilters.py +++ b/django/template/defaultfilters.py @@ -22,7 +22,7 @@ from django.utils.text import ( 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 index f02a8f61c86..1fcfa1fa884 100644 --- 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 index 3444c491db4..2ed9b321197 100644 --- 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 `. + +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 +`. diff --git a/docs/releases/3.2.11.txt b/docs/releases/3.2.11.txt index 621139033c2..e715ae866f2 100644 --- 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 `. + +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 +`. diff --git a/docs/releases/4.0.1.txt b/docs/releases/4.0.1.txt index 9429a8afffe..5335511f1ed 100644 --- 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 `. +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 +`. + Bugfixes ======== diff --git a/tests/template_tests/filter_tests/test_dictsort.py b/tests/template_tests/filter_tests/test_dictsort.py index 00c2bd42cbd..3de247fd86f 100644 --- 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 @@ class FunctionTests(SimpleTestCase): 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 @@ class FunctionTests(SimpleTestCase): 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 index ada199e127d..e2e24e31284 100644 --- a/tests/template_tests/filter_tests/test_dictsortreversed.py +++ b/tests/template_tests/filter_tests/test_dictsortreversed.py @@ -46,3 +46,9 @@ class FunctionTests(SimpleTestCase): 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'), '')