Opened 3 months ago

Closed 6 days ago

Last modified 6 days ago

#36795 closed Cleanup/optimization (fixed)

Always quote user-provided aliases

Reported by: Jacob Walls Owned by: Simon Charette
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Simon Charette Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

As part of evaluating CVE-2025-13372 (mitigated in 5b90ca1e7591fa36fccf2d6dad67cf1477e6293e), Simon Charette observed that we could reduce the potential for mistakes in this area by systematically quoting user-provided aliases, instead of checking aliases against FORBIDDEN_ALIAS_PATTERN:

... the root of the problem here is that we don't perform alias quoting when they are provided by the user as on some backends they are treated differently mainly regarding case (e.g. "foo" != "FOO" on Postgres). If we did systematically quote aliases most of the FORBIDDEN_ALIAS_PATTERN logic would be unnecessary (as long as we escape quotes in aliases) as aliases would always be quoted.

There is work to do and deprecation cycles to put in place (mainly regarding extra(...) usage) if we want to venture that way (particularly regarding subqueries) but try ... the following patch

  • django/db/models/expressions.py 

    diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py
index 0d47366d2c..c6eb9750e2 100644
    a b def __repr__(self):  
    13221322    def as_sql(self, compiler, connection):
    13231323        alias, column = self.alias, self.target.column
    13241324        identifiers = (alias, column) if alias else (column,)
    1325         sql = ".".join(map(compiler.quote_name_unless_alias, identifiers))
     1325        sql = ".".join(map(compiler.quote_name, identifiers))
    13261326        return sql, ()
    13271327
    13281328    def relabeled_clone(self, relabels):

  • django/db/models/sql/datastructures.py 

    diff --git a/django/db/models/sql/datastructures.py b/django/db/models/sql/datastructures.py
index 5314d37a1a..5ef5efbcfc 100644
    a b def as_sql(self, compiler, connection):  
    125125        sql = "%s %s%s ON (%s)" % (
    126126            self.join_type,
    127127            qn(self.table_name),
    128             alias_str,
     128            qn(alias_str),
    129129            on_clause_sql,
    130130        )
    131131        return sql, params

(Notice that is *not* a complete patch but rather a sketch to send a contributor down a fruitful path.)

With agreement from the Security Team, Simon's recommendation was that after the mitigation we should:

... investigate how much work it would be to treat user vs system generated aliases differently in a public ticket.

Change History (10)

comment:1 by Simon Charette, 3 months ago

Owner: set to Simon Charette
Status: new → assigned

Thanks for creating an actionable item out of this Jacob, I can self assign and look into it during the holidays.

comment:2 by Simon Charette, 3 months ago

Triage Stage: Unreviewed → Accepted

comment:3 by Rudraksha Dwivedi, 3 months ago

Owner: changed from Simon Charette to Rudraksha Dwivedi

comment:4 by Jacob Walls, 3 months ago

Owner: changed from Rudraksha Dwivedi to Simon Charette

comment:5 by Rudraksha Dwivedi, 3 months ago

Hey!
sorry about abruption here I genuinely thought I understood the Issue, clearly i didn't

comment:6 by Simon Charette, 10 days ago

Has patch: set

comment:7 by Jacob Walls, 8 days ago

Triage Stage: Accepted → Ready for checkin

comment:8 by Jacob Walls <jacobtylerwalls@…>, 6 days ago

Resolution: → fixed
Status: assigned → closed

In f05fac8:

Fixed #36795 -- Enforced quoting of all database object names.

This ensures all database identifiers are quoted independently of their orign
and most importantly that user provided aliases through annotate() and alias()
which paves the way for dropping the allow list of characters such aliases can
contain.

This will require adjustments to raw SQL interfaces such as RawSQL that might
make reference to ORM managed annotations as these will now be quoted.

The SQLCompiler.quote_name_unless_alias method is kept for now as an alias
for the newly introduced .quote_name method but will be duly deprecated in
a follow up commit.

comment:9 by Jacob Walls <jacobtylerwalls@…>, 6 days ago

In 5146449a:

Refs #36795 -- Removed unnecessary prohibits_dollar_signs_in_column_aliases feature flag.

Now that user provided aliases are systematically quoted there is no need to
disallow the usage of the dollar sign on Postgres.

comment:10 by Jacob Walls <jacobtylerwalls@…>, 6 days ago

In 1786cd88:

Refs #36795 -- Deprecated SQLCompiler.quote_name_unless_alias().

It has been superseded with .quote_name(), which ensures aliases are
always quoted.

Note: See TracTickets for help on using tickets.
Back to Top