[2.1.x] Fixed CVE-2019-14234 -- Protected JSONField/HStoreField key and index lookups against SQL injection.

felixxm · carltongibson · commit f74b3ae3628c · 2019-07-31T12:43:32.000+02:00
Thanks to Sage M. Abdullah for the report and initial patch.
Thanks Florian Apolloner for reviews.
diff --git a/django/contrib/postgres/fields/hstore.py b/django/contrib/postgres/fields/hstore.py
@@ -86,7 +86,7 @@ def __init__(self, key_name, *args, **kwargs):
 
     def as_sql(self, compiler, connection):
         lhs, params = compiler.compile(self.lhs)
-        return "(%s -> '%s')" % (lhs, self.key_name), params
+        return '(%s -> %%s)' % lhs, [self.key_name] + params
 
 
 class KeyTransformFactory:
diff --git a/django/contrib/postgres/fields/jsonb.py b/django/contrib/postgres/fields/jsonb.py
@@ -109,12 +109,10 @@ def as_sql(self, compiler, connection):
         if len(key_transforms) > 1:
             return "(%s %s %%s)" % (lhs, self.nested_operator), [key_transforms] + params
         try:
-            int(self.key_name)
+            lookup = int(self.key_name)
         except ValueError:
-            lookup = "'%s'" % self.key_name
-        else:
-            lookup = "%s" % self.key_name
-        return "(%s %s %s)" % (lhs, self.operator, lookup), params
+            lookup = self.key_name
+        return '(%s %s %%s)' % (lhs, self.operator), [lookup] + params
 
 
 class KeyTextTransform(KeyTransform):
diff --git a/docs/releases/1.11.23.txt b/docs/releases/1.11.23.txt
@@ -36,3 +36,12 @@ Remember that absolutely NO guarantee is provided about the results of
 ``strip_tags()`` being HTML safe. So NEVER mark safe the result of a
 ``strip_tags()`` call without escaping it first, for example with
 :func:`django.utils.html.escape`.
+
+CVE-2019-14234: SQL injection possibility in key and index lookups for ``JSONField``/``HStoreField``
+====================================================================================================
+
+:lookup:`Key and index lookups <jsonfield.key>` for
+:class:`~django.contrib.postgres.fields.JSONField` and :lookup:`key lookups
+<hstorefield.key>` for :class:`~django.contrib.postgres.fields.HStoreField`
+were subject to SQL injection, using a suitably crafted dictionary, with
+dictionary expansion, as the ``**kwargs`` passed to ``QuerySet.filter()``.
diff --git a/docs/releases/2.1.11.txt b/docs/releases/2.1.11.txt
@@ -36,3 +36,12 @@ Remember that absolutely NO guarantee is provided about the results of
 ``strip_tags()`` being HTML safe. So NEVER mark safe the result of a
 ``strip_tags()`` call without escaping it first, for example with
 :func:`django.utils.html.escape`.
+
+CVE-2019-14234: SQL injection possibility in key and index lookups for ``JSONField``/``HStoreField``
+====================================================================================================
+
+:lookup:`Key and index lookups <jsonfield.key>` for
+:class:`~django.contrib.postgres.fields.JSONField` and :lookup:`key lookups
+<hstorefield.key>` for :class:`~django.contrib.postgres.fields.HStoreField`
+were subject to SQL injection, using a suitably crafted dictionary, with
+dictionary expansion, as the ``**kwargs`` passed to ``QuerySet.filter()``.
diff --git a/tests/postgres_tests/test_hstore.py b/tests/postgres_tests/test_hstore.py
@@ -1,8 +1,11 @@
 import json
 
 from django.core import checks, exceptions, serializers
+from django.db import connection
 from django.forms import Form
-from django.test.utils import isolate_apps, modify_settings
+from django.test.utils import (
+    CaptureQueriesContext, isolate_apps, modify_settings,
+)
 
 from . import PostgreSQLTestCase
 from .models import HStoreModel, PostgreSQLModel
@@ -189,6 +192,18 @@ def test_usage_in_subquery(self):
             self.objs[:2]
         )
 
+    def test_key_sql_injection(self):
+        with CaptureQueriesContext(connection) as queries:
+            self.assertFalse(
+                HStoreModel.objects.filter(**{
+                    "field__test' = 'a') OR 1 = 1 OR ('d": 'x',
+                }).exists()
+            )
+        self.assertIn(
+            """."field" -> 'test'' = ''a'') OR 1 = 1 OR (''d') = 'x' """,
+            queries[0]['sql'],
+        )
+
 
 @isolate_apps('postgres_tests')
 class TestChecks(PostgreSQLTestCase):
diff --git a/tests/postgres_tests/test_json.py b/tests/postgres_tests/test_json.py
@@ -4,9 +4,10 @@
 
 from django.core import checks, exceptions, serializers
 from django.core.serializers.json import DjangoJSONEncoder
+from django.db import connection
 from django.db.models import Q
 from django.forms import CharField, Form, widgets
-from django.test.utils import isolate_apps
+from django.test.utils import CaptureQueriesContext, isolate_apps
 from django.utils.html import escape
 
 from . import PostgreSQLTestCase
@@ -299,6 +300,18 @@ def test_regex(self):
     def test_iregex(self):
         self.assertTrue(JSONModel.objects.filter(field__foo__iregex=r'^bAr$').exists())
 
+    def test_key_sql_injection(self):
+        with CaptureQueriesContext(connection) as queries:
+            self.assertFalse(
+                JSONModel.objects.filter(**{
+                    """field__test' = '"a"') OR 1 = 1 OR ('d""": 'x',
+                }).exists()
+            )
+        self.assertIn(
+            """."field" -> 'test'' = ''"a"'') OR 1 = 1 OR (''d') = '"x"' """,
+            queries[0]['sql'],
+        )
+
 
 @isolate_apps('postgres_tests')
 class TestChecks(PostgreSQLTestCase):
