Fixed CVE-2022-28347 -- Protected QuerySet.explain(**options) against SQL injection on PostgreSQL.

felixxm · felixxm · commit 6723a26e59b0 · 2022-04-11T08:59:58.000+02:00
diff --git a/django/db/backends/postgresql/features.py b/django/db/backends/postgresql/features.py
@@ -54,7 +54,6 @@ class DatabaseFeatures(BaseDatabaseFeatures):
     only_supports_unbounded_with_preceding_and_following = True
     supports_aggregate_filter_clause = True
     supported_explain_formats = {"JSON", "TEXT", "XML", "YAML"}
-    validates_explain_options = False  # A query will error on invalid options.
     supports_deferrable_unique_constraints = True
     has_json_operators = True
     json_key_contains_list_matching_requires_list = True
diff --git a/django/db/backends/postgresql/operations.py b/django/db/backends/postgresql/operations.py
@@ -9,6 +9,18 @@
 class DatabaseOperations(BaseDatabaseOperations):
     cast_char_field_without_max_length = "varchar"
     explain_prefix = "EXPLAIN"
+    explain_options = frozenset(
+        [
+            "ANALYZE",
+            "BUFFERS",
+            "COSTS",
+            "SETTINGS",
+            "SUMMARY",
+            "TIMING",
+            "VERBOSE",
+            "WAL",
+        ]
+    )
     cast_data_types = {
         "AutoField": "integer",
         "BigAutoField": "bigint",
@@ -298,17 +310,20 @@ def subtract_temporals(self, internal_type, lhs, rhs):
         return super().subtract_temporals(internal_type, lhs, rhs)
 
     def explain_query_prefix(self, format=None, **options):
-        prefix = super().explain_query_prefix(format)
         extra = {}
+        # Normalize options.
+        if options:
+            options = {
+                name.upper(): "true" if value else "false"
+                for name, value in options.items()
+            }
+            for valid_option in self.explain_options:
+                value = options.pop(valid_option, None)
+                if value is not None:
+                    extra[valid_option.upper()] = value
+        prefix = super().explain_query_prefix(format, **options)
         if format:
             extra["FORMAT"] = format
-        if options:
-            extra.update(
-                {
-                    name.upper(): "true" if value else "false"
-                    for name, value in options.items()
-                }
-            )
         if extra:
             prefix += " (%s)" % ", ".join("%s %s" % i for i in extra.items())
         return prefix
diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py
@@ -49,6 +49,10 @@
 # SQL comments are forbidden in column aliases.
 FORBIDDEN_ALIAS_PATTERN = _lazy_re_compile(r"['`\"\]\[;\s]|--|/\*|\*/")
 
+# Inspired from
+# https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS
+EXPLAIN_OPTIONS_PATTERN = _lazy_re_compile(r"[\w\-]+")
+
 
 def get_field_names_from_opts(opts):
     if opts is None:
@@ -589,6 +593,12 @@ def has_results(self, using):
 
     def explain(self, using, format=None, **options):
         q = self.clone()
+        for option_name in options:
+            if (
+                not EXPLAIN_OPTIONS_PATTERN.fullmatch(option_name)
+                or "--" in option_name
+            ):
+                raise ValueError(f"Invalid option name: {option_name!r}.")
         q.explain_info = ExplainInfo(format, options)
         compiler = q.get_compiler(using=using)
         return "\n".join(compiler.explain_query())
diff --git a/docs/releases/2.2.28.txt b/docs/releases/2.2.28.txt
@@ -13,3 +13,10 @@ CVE-2022-28346: Potential SQL injection in ``QuerySet.annotate()``, ``aggregate(
 :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.
+
+CVE-2022-28347: Potential SQL injection via ``QuerySet.explain(**options)`` on PostgreSQL
+=========================================================================================
+
+:meth:`.QuerySet.explain` method was subject to SQL injection in option names,
+using a suitably crafted dictionary, with dictionary expansion, as the
+``**options`` argument.
diff --git a/docs/releases/3.2.13.txt b/docs/releases/3.2.13.txt
@@ -15,6 +15,13 @@ CVE-2022-28346: Potential SQL injection in ``QuerySet.annotate()``, ``aggregate(
 aliases, using a suitably crafted dictionary, with dictionary expansion, as the
 ``**kwargs`` passed to these methods.
 
+CVE-2022-28347: Potential SQL injection via ``QuerySet.explain(**options)`` on PostgreSQL
+=========================================================================================
+
+:meth:`.QuerySet.explain` method was subject to SQL injection in option names,
+using a suitably crafted dictionary, with dictionary expansion, as the
+``**options`` argument.
+
 Bugfixes
 ========
 
diff --git a/docs/releases/4.0.4.txt b/docs/releases/4.0.4.txt
@@ -15,6 +15,13 @@ CVE-2022-28346: Potential SQL injection in ``QuerySet.annotate()``, ``aggregate(
 aliases, using a suitably crafted dictionary, with dictionary expansion, as the
 ``**kwargs`` passed to these methods.
 
+CVE-2022-28347: Potential SQL injection via ``QuerySet.explain(**options)`` on PostgreSQL
+=========================================================================================
+
+:meth:`.QuerySet.explain` method was subject to SQL injection in option names,
+using a suitably crafted dictionary, with dictionary expansion, as the
+``**options`` argument.
+
 Bugfixes
 ========
 
diff --git a/tests/queries/test_explain.py b/tests/queries/test_explain.py
@@ -59,8 +59,8 @@ def test_basic(self):
 
     @skipUnlessDBFeature("validates_explain_options")
     def test_unknown_options(self):
-        with self.assertRaisesMessage(ValueError, "Unknown options: test, test2"):
-            Tag.objects.explain(test=1, test2=1)
+        with self.assertRaisesMessage(ValueError, "Unknown options: TEST, TEST2"):
+            Tag.objects.explain(**{"TEST": 1, "TEST2": 1})
 
     def test_unknown_format(self):
         msg = "DOES NOT EXIST is not a recognized format."
@@ -94,6 +94,35 @@ def test_postgres_options(self):
                     option = "{} {}".format(name.upper(), "true" if value else "false")
                     self.assertIn(option, captured_queries[0]["sql"])
 
+    def test_option_sql_injection(self):
+        qs = Tag.objects.filter(name="test")
+        options = {"SUMMARY true) SELECT 1; --": True}
+        msg = "Invalid option name: 'SUMMARY true) SELECT 1; --'"
+        with self.assertRaisesMessage(ValueError, msg):
+            qs.explain(**options)
+
+    def test_invalid_option_names(self):
+        qs = Tag.objects.filter(name="test")
+        tests = [
+            'opt"ion',
+            "o'ption",
+            "op`tion",
+            "opti on",
+            "option--",
+            "optio\tn",
+            "o\nption",
+            "option;",
+            "你 好",
+            # [] are used by MSSQL.
+            "option[",
+            "option]",
+        ]
+        for invalid_option in tests:
+            with self.subTest(invalid_option):
+                msg = f"Invalid option name: {invalid_option!r}"
+                with self.assertRaisesMessage(ValueError, msg):
+                    qs.explain(**{invalid_option: True})
+
     @unittest.skipUnless(connection.vendor == "mysql", "MySQL specific")
     def test_mysql_text_to_traditional(self):
         # Ensure these cached properties are initialized to prevent queries for
