[2.2.x] Fixed CVE-2020-9402 -- Properly escaped tolerance parameter in GIS functions and aggregates on Oracle.

felixxm · felixxm · commit fe886a3b58a9 · 2020-03-04T09:34:39.000+01:00
Thanks to Norbert Szetei for the report.
diff --git a/django/contrib/gis/db/models/aggregates.py b/django/contrib/gis/db/models/aggregates.py
@@ -1,6 +1,7 @@
 from django.contrib.gis.db.models.fields import (
     ExtentField, GeometryCollectionField, GeometryField, LineStringField,
 )
+from django.db.models import Value
 from django.db.models.aggregates import Aggregate
 from django.utils.functional import cached_property
 
@@ -27,9 +28,16 @@ def as_sql(self, compiler, connection, function=None, **extra_context):
         )
 
     def as_oracle(self, compiler, connection, **extra_context):
-        tolerance = self.extra.get('tolerance') or getattr(self, 'tolerance', 0.05)
-        template = None if self.is_extent else '%(function)s(SDOAGGRTYPE(%(expressions)s,%(tolerance)s))'
-        return self.as_sql(compiler, connection, template=template, tolerance=tolerance, **extra_context)
+        if not self.is_extent:
+            tolerance = self.extra.get('tolerance') or getattr(self, 'tolerance', 0.05)
+            clone = self.copy()
+            clone.set_source_expressions([
+                *self.get_source_expressions(),
+                Value(tolerance),
+            ])
+            template = '%(function)s(SDOAGGRTYPE(%(expressions)s))'
+            return clone.as_sql(compiler, connection, template=template, **extra_context)
+        return self.as_sql(compiler, connection, **extra_context)
 
     def resolve_expression(self, query=None, allow_joins=True, reuse=None, summarize=False, for_save=False):
         c = super().resolve_expression(query, allow_joins, reuse, summarize, for_save)
diff --git a/django/contrib/gis/db/models/functions.py b/django/contrib/gis/db/models/functions.py
@@ -113,12 +113,14 @@ class OracleToleranceMixin:
     tolerance = 0.05
 
     def as_oracle(self, compiler, connection, **extra_context):
-        tol = self.extra.get('tolerance', self.tolerance)
-        return self.as_sql(
-            compiler, connection,
-            template="%%(function)s(%%(expressions)s, %s)" % tol,
-            **extra_context
-        )
+        tolerance = Value(self._handle_param(
+            self.extra.get('tolerance', self.tolerance),
+            'tolerance',
+            NUMERIC_TYPES,
+        ))
+        clone = self.copy()
+        clone.set_source_expressions([*self.get_source_expressions(), tolerance])
+        return clone.as_sql(compiler, connection, **extra_context)
 
 
 class Area(OracleToleranceMixin, GeoFunc):
diff --git a/docs/releases/1.11.29.txt b/docs/releases/1.11.29.txt
@@ -0,0 +1,13 @@
+============================
+Django 1.11.29 release notes
+============================
+
+*March 4, 2020*
+
+Django 1.11.29 fixes a security issue in 1.11.29.
+
+CVE-2020-9402: Potential SQL injection via ``tolerance`` parameter in GIS functions and aggregates on Oracle
+============================================================================================================
+
+GIS functions and aggregates on Oracle were subject to SQL injection,
+using a suitably crafted ``tolerance``.
diff --git a/docs/releases/2.2.11.txt b/docs/releases/2.2.11.txt
@@ -2,9 +2,15 @@
 Django 2.2.11 release notes
 ===========================
 
-*Expected March 2, 2020*
+*March 4, 2020*
 
-Django 2.2.11 fixes a data loss bug in 2.2.10.
+Django 2.2.11 fixes a security issue and a data loss bug in 2.2.10.
+
+CVE-2020-9402: Potential SQL injection via ``tolerance`` parameter in GIS functions and aggregates on Oracle
+============================================================================================================
+
+GIS functions and aggregates on Oracle were subject to SQL injection,
+using a suitably crafted ``tolerance``.
 
 Bugfixes
 ========
diff --git a/docs/releases/index.txt b/docs/releases/index.txt
@@ -85,6 +85,7 @@ versions of the documentation contain the release notes for any later releases.
 .. toctree::
    :maxdepth: 1
 
+   1.11.29
    1.11.28
    1.11.27
    1.11.26
diff --git a/tests/gis_tests/distapp/tests.py b/tests/gis_tests/distapp/tests.py
@@ -400,6 +400,37 @@ def test_distance_function_d_lookup(self):
         ).filter(d=D(m=1))
         self.assertTrue(qs.exists())
 
+    @unittest.skipUnless(
+        connection.vendor == 'oracle',
+        'Oracle supports tolerance paremeter.',
+    )
+    def test_distance_function_tolerance_escaping(self):
+        qs = Interstate.objects.annotate(
+            d=Distance(
+                Point(500, 500, srid=3857),
+                Point(0, 0, srid=3857),
+                tolerance='0.05) = 1 OR 1=1 OR (1+1',
+            ),
+        ).filter(d=D(m=1)).values('pk')
+        msg = 'The tolerance parameter has the wrong type'
+        with self.assertRaisesMessage(TypeError, msg):
+            qs.exists()
+
+    @unittest.skipUnless(
+        connection.vendor == 'oracle',
+        'Oracle supports tolerance paremeter.',
+    )
+    def test_distance_function_tolerance(self):
+        # Tolerance is greater than distance.
+        qs = Interstate.objects.annotate(
+            d=Distance(
+                Point(0, 0, srid=3857),
+                Point(1, 1, srid=3857),
+                tolerance=1.5,
+            ),
+        ).filter(d=0).values('pk')
+        self.assertIs(qs.exists(), True)
+
     @skipIfDBFeature("supports_distance_geodetic")
     @skipUnlessDBFeature("has_Distance_function")
     def test_distance_function_raw_result_d_lookup(self):
diff --git a/tests/gis_tests/geoapp/tests.py b/tests/gis_tests/geoapp/tests.py
@@ -1,4 +1,5 @@
 import tempfile
+import unittest
 from io import StringIO
 
 from django.contrib.gis import gdal
@@ -8,7 +9,7 @@
     MultiPoint, MultiPolygon, Point, Polygon, fromstr,
 )
 from django.core.management import call_command
-from django.db import NotSupportedError, connection
+from django.db import DatabaseError, NotSupportedError, connection
 from django.test import TestCase, skipUnlessDBFeature
 
 from ..utils import (
@@ -563,6 +564,42 @@ def test_unionagg(self):
         qs = City.objects.filter(name='NotACity')
         self.assertIsNone(qs.aggregate(Union('point'))['point__union'])
 
+    @unittest.skipUnless(
+        connection.vendor == 'oracle',
+        'Oracle supports tolerance paremeter.',
+    )
+    def test_unionagg_tolerance(self):
+        City.objects.create(
+            point=fromstr('POINT(-96.467222 32.751389)', srid=4326),
+            name='Forney',
+        )
+        tx = Country.objects.get(name='Texas').mpoly
+        # Tolerance is greater than distance between Forney and Dallas, that's
+        # why Dallas is ignored.
+        forney_houston = GEOSGeometry(
+            'MULTIPOINT(-95.363151 29.763374, -96.467222 32.751389)',
+            srid=4326,
+        )
+        self.assertIs(
+            forney_houston.equals(
+                City.objects.filter(point__within=tx).aggregate(
+                    Union('point', tolerance=32000),
+                )['point__union'],
+            ),
+            True,
+        )
+
+    @unittest.skipUnless(
+        connection.vendor == 'oracle',
+        'Oracle supports tolerance paremeter.',
+    )
+    def test_unionagg_tolerance_escaping(self):
+        tx = Country.objects.get(name='Texas').mpoly
+        with self.assertRaises(DatabaseError):
+            City.objects.filter(point__within=tx).aggregate(
+                Union('point', tolerance='0.05))), (((1'),
+            )
+
     def test_within_subquery(self):
         """
         Using a queryset inside a geo lookup is working (using a subquery)
