From 2c09e68ec911919360d5f8502cefc312f9e03c5d Mon Sep 17 00:00:00 2001 From: Mariusz Felisiak Date: Fri, 1 Apr 2022 08:10:22 +0200 Subject: [2.2.x] Fixed CVE-2022-28346 -- Protected QuerySet.annotate(), aggregate(), and extra() against SQL injection in column aliases. Thanks Splunk team: Preston Elder, Jacob Davis, Jacob Moore, Matt Hanson, David Briggs, and a security researcher: Danylo Dmytriiev (DDV_UA) for the report. Backport of 93cae5cb2f9a4ef1514cf1a41f714fef08005200 from main. --- django/db/models/sql/query.py | 14 +++++++++++++ docs/releases/2.2.28.txt | 8 ++++++++ tests/aggregation/tests.py | 9 ++++++++ tests/annotations/tests.py | 34 +++++++++++++++++++++++++++++++ tests/expressions/test_queryset_values.py | 9 ++++++++ tests/queries/tests.py | 9 ++++++++ 6 files changed, 83 insertions(+) diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py index b99f0e90ef..412e817f10 100644 --- a/django/db/models/sql/query.py +++ b/django/db/models/sql/query.py @@ -8,6 +8,7 @@ all about the internals of models in order to get the information it needs. """ import difflib import functools +import re from collections import Counter, OrderedDict, namedtuple from collections.abc import Iterator, Mapping from itertools import chain, count, product @@ -40,6 +41,10 @@ from django.utils.tree import Node __all__ = ['Query', 'RawQuery'] +# Quotation marks ('"`[]), whitespace characters, semicolons, or inline +# SQL comments are forbidden in column aliases. +FORBIDDEN_ALIAS_PATTERN = re.compile(r"['`\"\]\[;\s]|--|/\*|\*/") + def get_field_names_from_opts(opts): return set(chain.from_iterable( @@ -994,8 +999,16 @@ class Query: alias = seen[int_model] = join_info.joins[-1] return alias or seen[None] + def check_alias(self, alias): + if FORBIDDEN_ALIAS_PATTERN.search(alias): + raise ValueError( + "Column aliases cannot contain whitespace characters, quotation marks, " + "semicolons, or SQL comments." + ) + def add_annotation(self, annotation, alias, is_summary=False): """Add a single annotation expression to the Query.""" + self.check_alias(alias) annotation = annotation.resolve_expression(self, allow_joins=True, reuse=None, summarize=is_summary) self.append_annotation_mask([alias]) @@ -1873,6 +1886,7 @@ class Query: else: param_iter = iter([]) for name, entry in select.items(): + self.check_alias(name) entry = str(entry) entry_params = [] pos = entry.find("%s") diff --git a/docs/releases/2.2.28.txt b/docs/releases/2.2.28.txt index 0669e2d599..a894bddb3c 100644 --- a/docs/releases/2.2.28.txt +++ b/docs/releases/2.2.28.txt @@ -5,3 +5,11 @@ Django 2.2.28 release notes *April 11, 2022* Django 2.2.28 fixes two security issues with severity "high" in 2.2.27. + +CVE-2022-28346: Potential SQL injection in ``QuerySet.annotate()``, ``aggregate()``, and ``extra()`` +==================================================================================================== + +:meth:`.QuerySet.annotate`, :meth:`~.QuerySet.aggregate`, and +:meth:`~.QuerySet.extra` methods were subject to SQL injection in column +aliases, using a suitably crafted dictionary, with dictionary expansion, as the +``**kwargs`` passed to these methods. diff --git a/tests/aggregation/tests.py b/tests/aggregation/tests.py index 3820496c9f..501a18700b 100644 --- a/tests/aggregation/tests.py +++ b/tests/aggregation/tests.py @@ -1114,3 +1114,12 @@ class AggregateTestCase(TestCase): Book.objects.aggregate(is_book=True) with self.assertRaisesMessage(TypeError, msg % ', '.join([str(FloatField()), 'True'])): Book.objects.aggregate(FloatField(), Avg('price'), is_book=True) + + def test_alias_sql_injection(self): + crafted_alias = """injected_name" from "aggregation_author"; --""" + msg = ( + "Column aliases cannot contain whitespace characters, quotation marks, " + "semicolons, or SQL comments." + ) + with self.assertRaisesMessage(ValueError, msg): + Author.objects.aggregate(**{crafted_alias: Avg("age")}) diff --git a/tests/annotations/tests.py b/tests/annotations/tests.py index 021f59d2d7..27cd7ebfb8 100644 --- a/tests/annotations/tests.py +++ b/tests/annotations/tests.py @@ -598,3 +598,37 @@ class NonAggregateAnnotationTestCase(TestCase): total_books=Subquery(long_books_qs, output_field=IntegerField()), ).values('name') self.assertCountEqual(publisher_books_qs, [{'name': 'Sams'}, {'name': 'Morgan Kaufmann'}]) + + def test_alias_sql_injection(self): + crafted_alias = """injected_name" from "annotations_book"; --""" + msg = ( + "Column aliases cannot contain whitespace characters, quotation marks, " + "semicolons, or SQL comments." + ) + with self.assertRaisesMessage(ValueError, msg): + Book.objects.annotate(**{crafted_alias: Value(1)}) + + def test_alias_forbidden_chars(self): + tests = [ + 'al"ias', + "a'lias", + "ali`as", + "alia s", + "alias\t", + "ali\nas", + "alias--", + "ali/*as", + "alias*/", + "alias;", + # [] are used by MSSQL. + "alias[", + "alias]", + ] + msg = ( + "Column aliases cannot contain whitespace characters, quotation marks, " + "semicolons, or SQL comments." + ) + for crafted_alias in tests: + with self.subTest(crafted_alias): + with self.assertRaisesMessage(ValueError, msg): + Book.objects.annotate(**{crafted_alias: Value(1)}) diff --git a/tests/expressions/test_queryset_values.py b/tests/expressions/test_queryset_values.py index e264597968..0804531869 100644 --- a/tests/expressions/test_queryset_values.py +++ b/tests/expressions/test_queryset_values.py @@ -27,6 +27,15 @@ class ValuesExpressionsTests(TestCase): [{'salary': 10}, {'salary': 20}, {'salary': 30}], ) + def test_values_expression_alias_sql_injection(self): + crafted_alias = """injected_name" from "expressions_company"; --""" + msg = ( + "Column aliases cannot contain whitespace characters, quotation marks, " + "semicolons, or SQL comments." + ) + with self.assertRaisesMessage(ValueError, msg): + Company.objects.values(**{crafted_alias: F("ceo__salary")}) + def test_values_expression_group_by(self): # values() applies annotate() first, so values selected are grouped by # id, not firstname. diff --git a/tests/queries/tests.py b/tests/queries/tests.py index e72ecaa654..99ab57f4fc 100644 --- a/tests/queries/tests.py +++ b/tests/queries/tests.py @@ -1737,6 +1737,15 @@ class Queries5Tests(TestCase): 'bar %s' ) + def test_extra_select_alias_sql_injection(self): + crafted_alias = """injected_name" from "queries_note"; --""" + msg = ( + "Column aliases cannot contain whitespace characters, quotation marks, " + "semicolons, or SQL comments." + ) + with self.assertRaisesMessage(ValueError, msg): + Note.objects.extra(select={crafted_alias: "1"}) + class SelectRelatedTests(TestCase): def test_tickets_3045_3288(self): -- cgit v1.2.1