From 93cae5cb2f9a4ef1514cf1a41f714fef08005200 Mon Sep 17 00:00:00 2001
From: Mariusz Felisiak <felisiak.mariusz@gmail.com>
Date: Fri, 1 Apr 2022 08:10:22 +0200
Subject: [PATCH] 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.
---
django/db/models/sql/query.py | 14 ++++++++
docs/releases/2.2.28.txt | 8 +++++
docs/releases/3.2.13.txt | 8 +++++
docs/releases/4.0.4.txt | 8 +++++
tests/aggregation/tests.py | 9 +++++
tests/annotations/tests.py | 43 +++++++++++++++++++++++
tests/expressions/test_queryset_values.py | 9 +++++
tests/queries/tests.py | 9 +++++
8 files changed, 108 insertions(+)
diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py
index c07a4b342a..894aa7db4a 100644
--- a/django/db/models/sql/query.py
+++ b/django/db/models/sql/query.py
@@ -40,10 +40,15 @@ from django.db.models.sql.constants import INNER, LOUTER, ORDER_DIR, SINGLE
from django.db.models.sql.datastructures import BaseTable, Empty, Join, MultiJoin
from django.db.models.sql.where import AND, OR, ExtraWhere, NothingNode, WhereNode
from django.utils.functional import cached_property
+from django.utils.regex_helper import _lazy_re_compile
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 = _lazy_re_compile(r"['`\"\]\[;\s]|--|/\*|\*/")
+
def get_field_names_from_opts(opts):
if opts is None:
@@ -1091,8 +1096,16 @@ class Query(BaseExpression):
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, select=True):
"""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
)
@@ -2269,6 +2282,7 @@ class Query(BaseExpression):
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/docs/releases/3.2.13.txt b/docs/releases/3.2.13.txt
index c26a969f95..ee20aa2ca1 100644
--- a/docs/releases/3.2.13.txt
+++ b/docs/releases/3.2.13.txt
@@ -7,6 +7,14 @@ Django 3.2.13 release notes
Django 3.2.13 fixes two security issues with severity "high" in
3.2.12 and a regression in 3.2.4.
+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.
+
Bugfixes
========
diff --git a/docs/releases/4.0.4.txt b/docs/releases/4.0.4.txt
index 6873e78900..6c22788bd1 100644
--- a/docs/releases/4.0.4.txt
+++ b/docs/releases/4.0.4.txt
@@ -7,6 +7,14 @@ Django 4.0.4 release notes
Django 4.0.4 fixes two security issues with severity "high" and two bugs in
4.0.3.
+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.
+
Bugfixes
========
diff --git a/tests/aggregation/tests.py b/tests/aggregation/tests.py
index 3922478bf3..61da0ebfe7 100644
--- a/tests/aggregation/tests.py
+++ b/tests/aggregation/tests.py
@@ -2048,6 +2048,15 @@ class AggregateTestCase(TestCase):
)
self.assertEqual(len(qs), 6)
+ 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")})
+
def test_exists_extra_where_with_aggregate(self):
qs = Book.objects.annotate(
count=Count("id"),
diff --git a/tests/annotations/tests.py b/tests/annotations/tests.py
index 5106a377ac..52a268c4ae 100644
--- a/tests/annotations/tests.py
+++ b/tests/annotations/tests.py
@@ -1076,6 +1076,40 @@ class NonAggregateAnnotationTestCase(TestCase):
],
)
+ 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)})
+
class AliasTests(TestCase):
@classmethod
@@ -1339,3 +1373,12 @@ class AliasTests(TestCase):
with self.subTest(operation=operation):
with self.assertRaisesMessage(FieldError, msg):
getattr(qs, operation)("rating_alias")
+
+ 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.alias(**{crafted_alias: Value(1)})
diff --git a/tests/expressions/test_queryset_values.py b/tests/expressions/test_queryset_values.py
index 147f02fffb..0dba623167 100644
--- a/tests/expressions/test_queryset_values.py
+++ b/tests/expressions/test_queryset_values.py
@@ -34,6 +34,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 f9d2ebf98f..7b70a5ae0a 100644
--- a/tests/queries/tests.py
+++ b/tests/queries/tests.py
@@ -1898,6 +1898,15 @@ class Queries5Tests(TestCase):
Note.objects.extra(select={"foo": "'bar %%s'"})[0].foo, "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"})
+
def test_queryset_reuse(self):
# Using querysets doesn't mutate aliases.
authors = Author.objects.filter(Q(name="a1") | Q(name="nonexistent"))