[5.1.x] Fixed CVE-2024-53908 -- Prevented SQL injections in direct HasKeyLookup usage on Oracle.

charettes · sarahboyce · commit 6943d61818e6 · 2024-12-04T13:47:31.000+01:00
Thanks Seokchan Yoon for the report, and Mariusz Felisiak and Sarah
Boyce for the reviews.
diff --git a/django/db/models/fields/json.py b/django/db/models/fields/json.py
@@ -193,20 +193,18 @@ def compile_json_path_final_key(self, key_transform):
         # Compile the final key without interpreting ints as array elements.
         return ".%s" % json.dumps(key_transform)
 
-    def as_sql(self, compiler, connection, template=None):
+    def _as_sql_parts(self, compiler, connection):
         # Process JSON path from the left-hand side.
         if isinstance(self.lhs, KeyTransform):
-            lhs, lhs_params, lhs_key_transforms = self.lhs.preprocess_lhs(
+            lhs_sql, lhs_params, lhs_key_transforms = self.lhs.preprocess_lhs(
                 compiler, connection
             )
             lhs_json_path = compile_json_path(lhs_key_transforms)
         else:
-            lhs, lhs_params = self.process_lhs(compiler, connection)
+            lhs_sql, lhs_params = self.process_lhs(compiler, connection)
             lhs_json_path = "$"
-        sql = template % lhs
         # Process JSON path from the right-hand side.
         rhs = self.rhs
-        rhs_params = []
         if not isinstance(rhs, (list, tuple)):
             rhs = [rhs]
         for key in rhs:
@@ -217,24 +215,43 @@ def as_sql(self, compiler, connection, template=None):
             *rhs_key_transforms, final_key = rhs_key_transforms
             rhs_json_path = compile_json_path(rhs_key_transforms, include_root=False)
             rhs_json_path += self.compile_json_path_final_key(final_key)
-            rhs_params.append(lhs_json_path + rhs_json_path)
+            yield lhs_sql, lhs_params, lhs_json_path + rhs_json_path
+
+    def _combine_sql_parts(self, parts):
         # Add condition for each key.
         if self.logical_operator:
-            sql = "(%s)" % self.logical_operator.join([sql] * len(rhs_params))
-        return sql, tuple(lhs_params) + tuple(rhs_params)
+            return "(%s)" % self.logical_operator.join(parts)
+        return "".join(parts)
+
+    def as_sql(self, compiler, connection, template=None):
+        sql_parts = []
+        params = []
+        for lhs_sql, lhs_params, rhs_json_path in self._as_sql_parts(
+            compiler, connection
+        ):
+            sql_parts.append(template % (lhs_sql, "%s"))
+            params.extend(lhs_params + [rhs_json_path])
+        return self._combine_sql_parts(sql_parts), tuple(params)
 
     def as_mysql(self, compiler, connection):
         return self.as_sql(
-            compiler, connection, template="JSON_CONTAINS_PATH(%s, 'one', %%s)"
+            compiler, connection, template="JSON_CONTAINS_PATH(%s, 'one', %s)"
         )
 
     def as_oracle(self, compiler, connection):
-        sql, params = self.as_sql(
-            compiler, connection, template="JSON_EXISTS(%s, '%%s')"
-        )
-        # Add paths directly into SQL because path expressions cannot be passed
-        # as bind variables on Oracle.
-        return sql % tuple(params), []
+        template = "JSON_EXISTS(%s, '%s')"
+        sql_parts = []
+        params = []
+        for lhs_sql, lhs_params, rhs_json_path in self._as_sql_parts(
+            compiler, connection
+        ):
+            # Add right-hand-side directly into SQL because it cannot be passed
+            # as bind variables to JSON_EXISTS. It might result in invalid
+            # queries but it is assumed that it cannot be evaded because the
+            # path is JSON serialized.
+            sql_parts.append(template % (lhs_sql, rhs_json_path))
+            params.extend(lhs_params)
+        return self._combine_sql_parts(sql_parts), tuple(params)
 
     def as_postgresql(self, compiler, connection):
         if isinstance(self.rhs, KeyTransform):
@@ -246,7 +263,7 @@ def as_postgresql(self, compiler, connection):
 
     def as_sqlite(self, compiler, connection):
         return self.as_sql(
-            compiler, connection, template="JSON_TYPE(%s, %%s) IS NOT NULL"
+            compiler, connection, template="JSON_TYPE(%s, %s) IS NOT NULL"
         )
 
 
@@ -455,9 +472,9 @@ def as_oracle(self, compiler, connection):
         return "(NOT %s OR %s IS NULL)" % (sql, lhs), tuple(params) + tuple(lhs_params)
 
     def as_sqlite(self, compiler, connection):
-        template = "JSON_TYPE(%s, %%s) IS NULL"
+        template = "JSON_TYPE(%s, %s) IS NULL"
         if not self.rhs:
-            template = "JSON_TYPE(%s, %%s) IS NOT NULL"
+            template = "JSON_TYPE(%s, %s) IS NOT NULL"
         return HasKeyOrArrayIndex(self.lhs.lhs, self.lhs.key_name).as_sql(
             compiler,
             connection,
diff --git a/docs/releases/4.2.17.txt b/docs/releases/4.2.17.txt
@@ -22,3 +22,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-2024-53908: Potential SQL injection via ``HasKey(lhs, rhs)`` on Oracle
+==========================================================================
+
+Direct usage of the ``django.db.models.fields.json.HasKey`` lookup on Oracle
+was subject to SQL injection if untrusted data was used as a ``lhs`` value.
+
+Applications that use the :lookup:`has_key <jsonfield.has_key>` lookup through
+the ``__`` syntax are unaffected.
diff --git a/docs/releases/5.0.10.txt b/docs/releases/5.0.10.txt
@@ -22,3 +22,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-2024-53908: Potential SQL injection via ``HasKey(lhs, rhs)`` on Oracle
+==========================================================================
+
+Direct usage of the ``django.db.models.fields.json.HasKey`` lookup on Oracle
+was subject to SQL injection if untrusted data was used as a ``lhs`` value.
+
+Applications that use the :lookup:`has_key <jsonfield.has_key>` lookup through
+the ``__`` syntax are unaffected.
diff --git a/docs/releases/5.1.4.txt b/docs/releases/5.1.4.txt
@@ -23,6 +23,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-2024-53908: Potential SQL injection via ``HasKey(lhs, rhs)`` on Oracle
+==========================================================================
+
+Direct usage of the ``django.db.models.fields.json.HasKey`` lookup on Oracle
+was subject to SQL injection if untrusted data was used as a ``lhs`` value.
+
+Applications that use the :lookup:`has_key <jsonfield.has_key>` lookup through
+the ``__`` syntax are unaffected.
+
 Bugfixes
 ========
 
diff --git a/tests/model_fields/test_jsonfield.py b/tests/model_fields/test_jsonfield.py
@@ -29,6 +29,7 @@
 from django.db.models.expressions import RawSQL
 from django.db.models.fields.json import (
     KT,
+    HasKey,
     KeyTextTransform,
     KeyTransform,
     KeyTransformFactory,
@@ -582,6 +583,14 @@ def test_has_key_deep(self):
                     [expected],
                 )
 
+    def test_has_key_literal_lookup(self):
+        self.assertSequenceEqual(
+            NullableJSONModel.objects.filter(
+                HasKey(Value({"foo": "bar"}, JSONField()), "foo")
+            ).order_by("id"),
+            self.objs,
+        )
+
     def test_has_key_list(self):
         obj = NullableJSONModel.objects.create(value=[{"a": 1}, {"b": "x"}])
         tests = [
