SQL Injection via unquoted identifiers (aliases) in ORM-generated SQL

Proof-of-concept (conceptual PoC to illustrate the risk and fix): Environment: - Django 5.1.x (pre-fix) vs post-fix behavior demonstrated on a test project using a minimal model. - Database backend (e.g., PostgreSQL) with standard configuration. Idea: - Attack vector relies on an attacker-supplied alias (via annotate()/alias()) containing characters that would be misinterpreted by unquoted identifiers (e.g., spaces, semicolons, or SQL-style fragments). - Before the fix, such an alias could yield invalid SQL or, in the worst case, open a path for injection if the alias is injected into a larger SQL string without proper quoting. - After the fix, the alias is quoted consistently, preventing the attacker-supplied string from altering SQL structure. PoC steps (conceptual, safe demonstration): 1) Define a simple model: from django.db import models class Article(models.Model): title = models.CharField(max_length=200) 2) Construct an annotation with a potentially dangerous alias (contains a space and semicolon, simulating attacker-controlled input): alias_with_space = 'ali as' # contains a space alias_with_semicolon = 'ali;DROP' # contains a semicolon (simulated malicious input) 3) Build a queryset using annotate with the attacker-controlled alias via a dict (allowed because dict keys can be arbitrary strings): qs = Article.objects.annotate(**{alias_with_space: models.F('title')}) sql_with_space_alias = str(qs.query) print(sql_with_space_alias) qs2 = Article.objects.annotate(**{alias_with_semicolon: models.F('title')}) sql_with_semicolon_alias = str(qs2.query) print(sql_with_semicolon_alias) 4) Expected behavior (post-fix): - The generated SQL should quote the aliases as needed, e.g. AS "ali as" or AS "ali;DROP" within the statement, ensuring the alias name cannot disrupt SQL syntax or enable injection. - The output SQL should include properly quoted identifiers rather than raw, unquoted identifiers. 5) What to observe: - On a Django build with this fix, the values printed in sql_with_space_alias and sql_with_semicolon_alias should show quoted aliases (e.g., AS "ali as", AS "ali;DROP"). - On an older build that did not apply this fix, you might see invalid SQL fragments like AS ali as or AS ali;DROP, which could cause SQL syntax errors or unintended behavior. Note: This PoC focuses on illustrating the alias-quoting behavior and potential attack surface involving attacker-controlled aliases. In real-world usage, you should run this in a safe test environment and compare the output SQL between versions to confirm quoting is applied consistently.

diff --git a/django/db/backends/mysql/compiler.py b/django/db/backends/mysql/compiler.py index 0291b76c7062..18c60868fd75 100644 --- a/django/db/backends/mysql/compiler.py +++ b/django/db/backends/mysql/compiler.py @@ -28,10 +28,7 @@ def as_sql(self): # window functions as it doesn't allow for GROUP BY/HAVING clauses # and the subquery wrapping (necessary to emulate QUALIFY). return super().as_sql() - result = [ - "DELETE %s FROM" - % self.quote_name_unless_alias(self.query.get_initial_alias()) - ] + result = ["DELETE %s FROM" % self.quote_name(self.query.get_initial_alias())] from_sql, params = self.get_from_clause() result.extend(from_sql) try: diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py index c6ba5c89a92e..0c58e7749c24 100644 --- a/django/db/models/expressions.py +++ b/django/db/models/expressions.py @@ -1366,7 +1366,7 @@ def __repr__(self): def as_sql(self, compiler, connection): alias, column = self.alias, self.target.column identifiers = (alias, column) if alias else (column,) - sql = ".".join(map(compiler.quote_name_unless_alias, identifiers)) + sql = ".".join(map(compiler.quote_name, identifiers)) return sql, () def relabeled_clone(self, relabels): diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py index 6c758fb5261a..f52ca515f29a 100644 --- a/django/db/models/sql/compiler.py +++ b/django/db/models/sql/compiler.py @@ -439,9 +439,7 @@ def _order_by_pairs(self): table, col = col.split(".", 1) yield ( OrderBy( - RawSQL( - "%s.%s" % (self.quote_name_unless_alias(table), col), [] - ), + RawSQL("%s.%s" % (self.quote_name(table), col), []), descending=descending, ), False, @@ -547,35 +545,19 @@ def get_extra_select(self, order_by, select): extra_select.append((expr, (without_ordering, params), None)) return extra_select - def quote_name_unless_alias(self, name): + def quote_name(self, name): """ - A wrapper around connection.ops.quote_name that doesn't quote aliases - for table names. This avoids problems with some SQL dialects that treat - quoted strings specially (e.g. PostgreSQL). + A wrapper around connection.ops.quote_name that memoizes quoted + name values. """ - if ( - self.connection.features.prohibits_dollar_signs_in_column_aliases - and "$" in name - ): - raise ValueError( - "Dollar signs are not permitted in column aliases on " - f"{self.connection.display_name}." - ) - if name in self.quote_cache: - return self.quote_cache[name] - if ( - (name in self.query.alias_map and name not in self.query.table_map) - or name in self.query.extra_select - or ( - self.query.external_aliases.get(name) - and name not in self.query.table_map - ) - ): - self.quote_cache[name] = name - return name - r = self.connection.ops.quote_name(name) - self.quote_cache[name] = r - return r + if (quoted := self.quote_cache.get(name)) is not None: + return quoted + quoted = self.connection.ops.quote_name(name) + self.quote_cache[name] = quoted + return quoted + + # Kept for backward compatiblity until duly done deprecation. + quote_name_unless_alias = quote_name def compile(self, node): vendor_impl = getattr(node, "as_" + self.connection.vendor, None) @@ -1175,7 +1157,7 @@ def get_from_clause(self): alias not in self.query.alias_map or self.query.alias_refcount[alias] == 1 ): - result.append(", %s" % self.quote_name_unless_alias(alias)) + result.append(", %s" % self.quote_name(alias)) return result, params def get_related_selections( @@ -1504,7 +1486,7 @@ def _get_field_choices(): if self.connection.features.select_for_update_of_column: result.append(self.compile(col)[0]) else: - result.append(self.quote_name_unless_alias(col.alias)) + result.append(self.quote_name(col.alias)) if invalid_names: raise FieldError( "Invalid field name(s) given in select_for_update(of=(...)): %s. " @@ -1805,9 +1787,7 @@ def assemble_as_sql(self, fields, value_rows): return placeholder_rows, param_rows def as_sql(self): - # We don't need quote_name_unless_alias() here, since these are all - # going to be column names (so we can avoid the extra overhead). - qn = self.connection.ops.quote_name + qn = self.quote_name opts = self.query.get_meta() insert_statement = self.connection.ops.insert_statement( on_conflict=self.query.on_conflict, @@ -2006,7 +1986,7 @@ def contains_self_reference_subquery(self): ) def _as_sql(self, query): - delete = "DELETE FROM %s" % self.quote_name_unless_alias(query.base_table) + delete = "DELETE FROM %s" % self.quote_name(query.base_table) try: where, params = self.compile(query.where) except FullResultSet: @@ -2050,7 +2030,7 @@ def as_sql(self): self.pre_sql_setup() if not self.query.values: return "", () - qn = self.quote_name_unless_alias + qn = self.quote_name values, update_params = [], [] for field, model, val in self.query.values: if hasattr(val, "resolve_expression"): diff --git a/django/db/models/sql/datastructures.py b/django/db/models/sql/datastructures.py index 5314d37a1ae3..b4eea2320fef 100644 --- a/django/db/models/sql/datastructures.py +++ b/django/db/models/sql/datastructures.py @@ -82,7 +82,7 @@ def as_sql(self, compiler, connection): """ join_conditions = [] params = [] - qn = compiler.quote_name_unless_alias + qn = compiler.quote_name # Add a join condition for each pair of joining columns. for lhs, rhs in self.join_fields: lhs, rhs = connection.ops.prepare_join_on_clause( @@ -120,7 +120,9 @@ def as_sql(self, compiler, connection): ) on_clause_sql = " AND ".join(join_conditions) alias_str = ( - "" if self.table_alias == self.table_name else (" %s" % self.table_alias) + "" + if self.table_alias == self.table_name + else (" %s" % qn(self.table_alias)) ) sql = "%s %s%s ON (%s)" % ( self.join_type, @@ -193,10 +195,13 @@ def __init__(self, table_name, alias): self.table_alias = alias def as_sql(self, compiler, connection): + qn = compiler.quote_name alias_str = ( - "" if self.table_alias == self.table_name else (" %s" % self.table_alias) + "" + if self.table_alias == self.table_name + else (" %s" % qn(self.table_alias)) ) - base_sql = compiler.quote_name_unless_alias(self.table_name) + base_sql = qn(self.table_name) return base_sql + alias_str, [] def relabeled_clone(self, change_map): diff --git a/docs/releases/6.1.txt b/docs/releases/6.1.txt index 1bd4f091aa39..56b11deb805f 100644 --- a/docs/releases/6.1.txt +++ b/docs/releases/6.1.txt @@ -434,6 +434,16 @@ backends. instead of the JSON ``null`` primitive. This matches the behavior of a standalone :class:`~django.db.models.JSONField` when storing ``None`` values. +Models +------ + +* SQL ``SELECT`` aliases originating from :meth:`.QuerySet.annotate` + calls as well as table and ``JOIN`` aliases are now systematically quoted to + prevent special character collisions. Because quoted aliases are + case-sensitive, *raw* SQL references to aliases mixing case, such as when + using :class:`.RawSQL`, might have to be adjusted to also make use of + quoting. + System checks ------------- diff --git a/tests/filtered_relation/tests.py b/tests/filtered_relation/tests.py index d15dd0d5f60f..9047feba2d01 100644 --- a/tests/filtered_relation/tests.py +++ b/tests/filtered_relation/tests.py @@ -210,8 +210,9 @@ def test_internal_queryset_alias_mapping(self): ), ).filter(book_alice__isnull=False) self.assertIn( - "INNER JOIN {} book_alice ON".format( - connection.ops.quote_name("filtered_relation_book") + "INNER JOIN {} {} ON".format( + connection.ops.quote_name("filtered_relation_book"), + connection.ops.quote_name("book_alice"), ), str(queryset.query), ) diff --git a/tests/foreign_object/models/article.py b/tests/foreign_object/models/article.py index 276296c8d49a..9d8a35da7c99 100644 --- a/tests/foreign_object/models/article.py +++ b/tests/foreign_object/models/article.py @@ -22,7 +22,7 @@ def __init__(self, alias, col, value): self.alias, self.col, self.value = alias, col, value def as_sql(self, compiler, connection): - qn = compiler.quote_name_unless_alias + qn = compiler.quote_name return "%s.%s = %%s" % (qn(self.alias), qn(self.col)), [self.value] diff --git a/tests/queries/tests.py b/tests/queries/tests.py index af657b25802a..d58eccaa12e4 100644 --- a/tests/queries/tests.py +++ b/tests/queries/tests.py @@ -195,7 +195,8 @@ def test_subquery_condition(self): # It is possible to reuse U for the second subquery, no need to use W. self.assertNotIn("w0", str(qs4.query).lower()) # So, 'U0."id"' is referenced in SELECT and WHERE twice. - self.assertEqual(str(qs4.query).lower().count("u0."), 4) + id_col = "%s." % connection.ops.quote_name("u0").lower() + self.assertEqual(str(qs4.query).lower().count(id_col), 4) def test_ticket1050(self): self.assertSequenceEqual(