From 29a6c98b4c13af82064f993f0acc6e8fafa4d3f5 Mon Sep 17 00:00:00 2001 From: Mariusz Felisiak Date: Fri, 1 Apr 2022 13:48:47 +0200 Subject: [PATCH] [2.2.x] Fixed CVE-2022-28347 -- Protected QuerySet.explain(**options) against SQL injection on PostgreSQL. Backport of 6723a26e59b0b5429a0c5873941e01a2e1bdbb81 from main. --- django/db/backends/postgresql/features.py | 1 - django/db/backends/postgresql/operations.py | 27 +++++++++++++---- django/db/models/sql/query.py | 10 +++++++ docs/releases/2.2.28.txt | 7 +++++ tests/queries/test_explain.py | 33 +++++++++++++++++++-- 5 files changed, 70 insertions(+), 8 deletions(-) diff --git a/django/db/backends/postgresql/features.py b/django/db/backends/postgresql/features.py index 5c8701c396d4..9f63ca6b0ce1 100644 --- a/django/db/backends/postgresql/features.py +++ b/django/db/backends/postgresql/features.py @@ -53,7 +53,6 @@ class DatabaseFeatures(BaseDatabaseFeatures): supports_over_clause = True supports_aggregate_filter_clause = True supported_explain_formats = {'JSON', 'TEXT', 'XML', 'YAML'} - validates_explain_options = False # A query will error on invalid options. @cached_property def is_postgresql_9_5(self): diff --git a/django/db/backends/postgresql/operations.py b/django/db/backends/postgresql/operations.py index 66e5482be6ba..66ac2d5d108c 100644 --- a/django/db/backends/postgresql/operations.py +++ b/django/db/backends/postgresql/operations.py @@ -8,6 +8,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', @@ -267,15 +279,20 @@ def window_frame_range_start_end(self, start=None, end=None): return start_, end_ def explain_query_prefix(self, format=None, **options): - prefix = super().explain_query_prefix(format) extra = {} - if format: - extra['FORMAT'] = format + # Normalize options. if options: - extra.update({ + 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 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 412e817f107e..1e823cfe74b1 100644 --- a/django/db/models/sql/query.py +++ b/django/db/models/sql/query.py @@ -45,6 +45,10 @@ # SQL comments are forbidden in column aliases. FORBIDDEN_ALIAS_PATTERN = re.compile(r"['`\"\]\[;\s]|--|/\*|\*/") +# Inspired from +# https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS +EXPLAIN_OPTIONS_PATTERN = re.compile(r"[\w\-]+") + def get_field_names_from_opts(opts): return set(chain.from_iterable( @@ -528,6 +532,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("Invalid option name: '%s'." % option_name) q.explain_query = True q.explain_format = format q.explain_options = options diff --git a/tests/queries/test_explain.py b/tests/queries/test_explain.py index 9428bd88e9c3..209c1923071e 100644 --- a/tests/queries/test_explain.py +++ b/tests/queries/test_explain.py @@ -41,8 +41,8 @@ def test_basic(self): @skipUnlessDBFeature('validates_explain_options') def test_unknown_options(self): - with self.assertRaisesMessage(ValueError, 'Unknown options: test, test2'): - Tag.objects.all().explain(test=1, test2=1) + with self.assertRaisesMessage(ValueError, "Unknown options: TEST, TEST2"): + Tag.objects.all().explain(**{"TEST": 1, "TEST2": 1}) def test_unknown_format(self): msg = 'DOES NOT EXIST is not a recognized format.' @@ -71,6 +71,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 = "Invalid option name: '%s'" % invalid_option + with self.assertRaisesMessage(ValueError, msg): + qs.explain(**{invalid_option: True}) + @unittest.skipUnless(connection.vendor == 'mysql', 'MySQL specific') def test_mysql_text_to_traditional(self): # Initialize the cached property, if needed, to prevent a query for