From 6723a26e59b0b5429a0c5873941e01a2e1bdbb81 Mon Sep 17 00:00:00 2001 From: Mariusz Felisiak Date: Fri, 1 Apr 2022 13:48:47 +0200 Subject: [PATCH] Fixed CVE-2022-28347 -- Protected QuerySet.explain(**options) against SQL injection on PostgreSQL. --- django/db/backends/postgresql/features.py | 1 - django/db/backends/postgresql/operations.py | 31 ++++++++++++++----- django/db/models/sql/query.py | 10 +++++++ docs/releases/2.2.28.txt | 7 +++++ docs/releases/3.2.13.txt | 7 +++++ docs/releases/4.0.4.txt | 7 +++++ tests/queries/test_explain.py | 33 +++++++++++++++++++-- 7 files changed, 85 insertions(+), 11 deletions(-) diff --git a/django/db/backends/postgresql/features.py b/django/db/backends/postgresql/features.py index 5e6752b97a..8aae4caf34 100644 --- 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 index 946baea212..ab451ac63f 100644 --- a/django/db/backends/postgresql/operations.py +++ b/django/db/backends/postgresql/operations.py @@ -9,6 +9,18 @@ from django.db.models.constants import OnConflict 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 @@ class DatabaseOperations(BaseDatabaseOperations): 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 index 894aa7db4a..a55eb84a17 100644 --- a/django/db/models/sql/query.py +++ b/django/db/models/sql/query.py @@ -49,6 +49,10 @@ __all__ = ["Query", "RawQuery"] # 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 @@ class Query(BaseExpression): 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 index a894bddb3c..43270fc5c0 100644 --- 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 index ee20aa2ca1..b7afbb8ed7 100644 --- 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 index 6c22788bd1..702cebbbe9 100644 --- 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 index ab845f992a..9eb4347323 100644 --- a/tests/queries/test_explain.py +++ b/tests/queries/test_explain.py @@ -59,8 +59,8 @@ class ExplainTests(TestCase): @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 @@ class ExplainTests(TestCase): 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