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

Thanks to Sage M. Abdullah for the report and initial patch.
Thanks Florian Apolloner for reviews.

7 files changed, 59 insertions, 8 deletions

diff --git a/django/contrib/postgres/fields/hstore.py b/django/contrib/postgres/fields/hstore.py
index 39f074b763..8d6cd6c812 100644
--- a/django/contrib/postgres/fields/hstore.py
+++ b/django/contrib/postgres/fields/hstore.py
@@ -86,7 +86,7 @@ class KeyTransform(Transform):
 
     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
index 966e8f1141..be98ff2d48 100644
--- a/django/contrib/postgres/fields/jsonb.py
+++ b/django/contrib/postgres/fields/jsonb.py
@@ -109,12 +109,10 @@ class KeyTransform(Transform):
         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
index c95ffd9a50..03b33ebf63 100644
--- 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
index 9cae1e6f2e..0de4175b5f 100644
--- 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/docs/releases/2.2.4.txt b/docs/releases/2.2.4.txt
index c965373677..3aac51869c 100644
--- a/docs/releases/2.2.4.txt
+++ b/docs/releases/2.2.4.txt
@@ -37,6 +37,15 @@ Remember that absolutely NO guarantee is provided about the results of
 ``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()``.
+
 Bugfixes
 ========
 
diff --git a/tests/postgres_tests/test_hstore.py b/tests/postgres_tests/test_hstore.py
index 1d7403fb20..29936e297e 100644
--- a/tests/postgres_tests/test_hstore.py
+++ b/tests/postgres_tests/test_hstore.py
@@ -1,8 +1,9 @@
 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
+from django.test.utils import CaptureQueriesContext, isolate_apps
 
 from . import PostgreSQLSimpleTestCase, PostgreSQLTestCase
 from .models import HStoreModel, PostgreSQLModel
@@ -185,6 +186,18 @@ class TestQuerying(PostgreSQLTestCase):
             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(PostgreSQLSimpleTestCase):
diff --git a/tests/postgres_tests/test_json.py b/tests/postgres_tests/test_json.py
index 9acd7c8bf8..c208df1b9f 100644
--- a/tests/postgres_tests/test_json.py
+++ b/tests/postgres_tests/test_json.py
@@ -5,9 +5,10 @@ from decimal import Decimal
 
 from django.core import checks, exceptions, serializers
 from django.core.serializers.json import DjangoJSONEncoder
+from django.db import connection
 from django.db.models import Count, 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 PostgreSQLSimpleTestCase, PostgreSQLTestCase
@@ -322,6 +323,18 @@ class TestQuerying(PostgreSQLTestCase):
     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(PostgreSQLSimpleTestCase):