diff --git a/django/contrib/gis/db/models/aggregates.py b/django/contrib/gis/db/models/aggregates.py index 416481f9cac9..0e077550a2e1 100644 --- a/django/contrib/gis/db/models/aggregates.py +++ b/django/contrib/gis/db/models/aggregates.py @@ -1,4 +1,5 @@ from django.contrib.gis.db.models.fields import ExtentField +from django.db.models import Value from django.db.models.aggregates import Aggregate __all__ = ['Collect', 'Extent', 'Extent3D', 'MakeLine', 'Union'] @@ -16,11 +17,14 @@ def as_sql(self, compiler, connection): return super(GeoAggregate, self).as_sql(compiler, connection) def as_oracle(self, compiler, connection): - if not hasattr(self, 'tolerance'): - self.tolerance = 0.05 - self.extra['tolerance'] = self.tolerance if not self.is_extent: - self.template = '%(function)s(SDOAGGRTYPE(%(expressions)s,%(tolerance)s))' + tolerance = self.extra.get('tolerance') or getattr(self, 'tolerance', 0.05) + clone = self.copy() + expressions = clone.get_source_expressions() + expressions.append(Value(tolerance)) + clone.set_source_expressions(expressions) + clone.template = '%(function)s(SDOAGGRTYPE(%(expressions)s))' + return clone.as_sql(compiler, connection) return self.as_sql(compiler, connection) def resolve_expression(self, query=None, allow_joins=True, reuse=None, summarize=False, for_save=False): diff --git a/django/contrib/gis/db/models/functions.py b/django/contrib/gis/db/models/functions.py index 95eda246ade1..ef9e11818944 100644 --- a/django/contrib/gis/db/models/functions.py +++ b/django/contrib/gis/db/models/functions.py @@ -117,7 +117,11 @@ class OracleToleranceMixin(object): tolerance = 0.05 def as_oracle(self, compiler, connection): - tol = self.extra.get('tolerance', self.tolerance) + tol = self._handle_param( + self.extra.get('tolerance', self.tolerance), + 'tolerance', + NUMERIC_TYPES, + ) self.template = "%%(function)s(%%(expressions)s, %s)" % tol return super(OracleToleranceMixin, self).as_sql(compiler, connection) diff --git a/django/contrib/postgres/aggregates/general.py b/django/contrib/postgres/aggregates/general.py index 5b3d22bf9817..cd9a4bc34988 100644 --- a/django/contrib/postgres/aggregates/general.py +++ b/django/contrib/postgres/aggregates/general.py @@ -1,4 +1,5 @@ from django.contrib.postgres.fields import JSONField +from django.db.models import Value from django.db.models.aggregates import Aggregate __all__ = [ @@ -43,11 +44,12 @@ def convert_value(self, value, expression, connection, context): class StringAgg(Aggregate): function = 'STRING_AGG' - template = "%(function)s(%(distinct)s%(expressions)s, '%(delimiter)s')" + template = '%(function)s(%(distinct)s%(expressions)s)' def __init__(self, expression, delimiter, distinct=False, **extra): distinct = 'DISTINCT ' if distinct else '' - super(StringAgg, self).__init__(expression, delimiter=delimiter, distinct=distinct, **extra) + delimiter_expr = Value(str(delimiter)) + super(StringAgg, self).__init__(expression, delimiter_expr, distinct=distinct, **extra) def convert_value(self, value, expression, connection, context): if not value: diff --git a/docs/releases/1.11.28.txt b/docs/releases/1.11.28.txt new file mode 100644 index 000000000000..81ccb0ce06f8 --- /dev/null +++ b/docs/releases/1.11.28.txt @@ -0,0 +1,13 @@ +============================ +Django 1.11.28 release notes +============================ + +*February 3, 2020* + +Django 1.11.28 fixes a security issue in 1.11.27. + +CVE-2020-7471: Potential SQL injection via ``StringAgg(delimiter)`` +=================================================================== + +:class:`~django.contrib.postgres.aggregates.StringAgg` aggregation function was +subject to SQL injection, using a suitably crafted ``delimiter``. diff --git a/docs/releases/1.11.29.txt b/docs/releases/1.11.29.txt new file mode 100644 index 000000000000..d37f3ffc0dac --- /dev/null +++ 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/index.txt b/docs/releases/index.txt index cd13f8695ed4..be5fb3e54e9a 100644 --- a/docs/releases/index.txt +++ b/docs/releases/index.txt @@ -26,6 +26,8 @@ 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 1.11.25 diff --git a/tests/gis_tests/distapp/tests.py b/tests/gis_tests/distapp/tests.py index 4609083f3b41..dd7f9efe69cc 100644 --- a/tests/gis_tests/distapp/tests.py +++ b/tests/gis_tests/distapp/tests.py @@ -1,6 +1,6 @@ from __future__ import unicode_literals -from unittest import skipIf +from unittest import skipIf, skipUnless from django.contrib.gis.db.models.functions import ( Area, Distance, Length, Perimeter, Transform, @@ -588,6 +588,37 @@ def test_distance_geodetic_spheroid(self): for i, c in enumerate(qs): self.assertAlmostEqual(sphere_distances[i], c.distance.m, tol) + @skipUnless( + connection.vendor == 'oracle', + 'Oracle supports tolerance paremeter.', + ) + def test_distance_function_tolerance_escaping(self): + qs = AustraliaCity.objects.annotate( + d=Distance( + 'point', + Point(0, 0, srid=3857), + tolerance='0.05) = 1 OR 1=1 OR (1+1', + ), + ).filter(d=1).values('pk') + msg = 'The tolerance parameter has the wrong type' + with self.assertRaisesMessage(TypeError, msg): + qs.exists() + + @skipUnless( + connection.vendor == 'oracle', + 'Oracle supports tolerance paremeter.', + ) + def test_distance_function_tolerance(self): + # Tolerance is greater than distance. + qs = AustraliaCity.objects.annotate( + d=Distance( + 'point', + Point(151.23, -33.95, srid=4326), + tolerance=340.7, + ), + ).filter(d=0).values('pk') + self.assertIs(qs.exists(), True) + @no_oracle # Oracle already handles geographic distance calculation. @skipUnlessDBFeature("has_Distance_function", 'has_Transform_function') def test_distance_transform(self): diff --git a/tests/gis_tests/geoapp/tests.py b/tests/gis_tests/geoapp/tests.py index b6a5c2d0c851..7eff87693d4a 100644 --- a/tests/gis_tests/geoapp/tests.py +++ b/tests/gis_tests/geoapp/tests.py @@ -2,6 +2,7 @@ import re import tempfile +import unittest from django.contrib.gis import gdal from django.contrib.gis.db.models import Extent, MakeLine, Union @@ -10,7 +11,7 @@ MultiPoint, MultiPolygon, Point, Polygon, fromstr, ) from django.core.management import call_command -from django.db import connection +from django.db import DatabaseError, connection from django.test import TestCase, ignore_warnings, skipUnlessDBFeature from django.utils import six from django.utils.deprecation import RemovedInDjango20Warning @@ -881,6 +882,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) diff --git a/tests/postgres_tests/test_aggregates.py b/tests/postgres_tests/test_aggregates.py index 9aa0e0659577..ddfd7fce267f 100644 --- a/tests/postgres_tests/test_aggregates.py +++ b/tests/postgres_tests/test_aggregates.py @@ -108,6 +108,10 @@ def test_string_agg_requires_delimiter(self): with self.assertRaises(TypeError): AggregateTestModel.objects.aggregate(stringagg=StringAgg('char_field')) + def test_string_agg_delimiter_escaping(self): + values = AggregateTestModel.objects.aggregate(stringagg=StringAgg('char_field', delimiter="'")) + self.assertEqual(values, {'stringagg': "Foo1'Foo2'Foo3'Foo4"}) + def test_string_agg_charfield(self): values = AggregateTestModel.objects.aggregate(stringagg=StringAgg('char_field', delimiter=';')) self.assertEqual(values, {'stringagg': 'Foo1;Foo2;Foo3;Foo4'})