From 75c0d4ea3ae48970f788c482ee0bd6b29a7f1307 Mon Sep 17 00:00:00 2001 From: Erik Romijn Date: Sun, 20 Apr 2014 16:13:41 -0400 Subject: [PATCH] Fixed queries that may return unexpected results on MySQL due to typecasting. This is a security fix; disclosure to follow shortly. --- django/db/models/fields/__init__.py | 16 +++++++++++++- docs/howto/custom-model-fields.txt | 11 ++++++++++ docs/ref/databases.txt | 16 ++++++++++++++ docs/ref/models/querysets.txt | 10 +++++++++ docs/topics/db/sql.txt | 10 +++++++++ tests/model_fields/tests.py | 34 ++++++++++++++++++++++++++++- 6 files changed, 95 insertions(+), 2 deletions(-) diff --git a/django/db/models/fields/__init__.py b/django/db/models/fields/__init__.py index 7bac2c3869..caf0311cc3 100644 --- a/django/db/models/fields/__init__.py +++ b/django/db/models/fields/__init__.py @@ -1509,6 +1509,12 @@ class FilePathField(Field): del kwargs["max_length"] return name, path, args, kwargs + def get_prep_value(self, value): + value = super(FilePathField, self).get_prep_value(value) + if value is None: + return None + return six.text_type(value) + def formfield(self, **kwargs): defaults = { 'path': self.path, @@ -1642,6 +1648,12 @@ class IPAddressField(Field): del kwargs['max_length'] return name, path, args, kwargs + def get_prep_value(self, value): + value = super(IPAddressField, self).get_prep_value(value) + if value is None: + return None + return six.text_type(value) + def get_internal_type(self): return "IPAddressField" @@ -1711,12 +1723,14 @@ class GenericIPAddressField(Field): def get_prep_value(self, value): value = super(GenericIPAddressField, self).get_prep_value(value) + if value is None: + return None if value and ':' in value: try: return clean_ipv6_address(value, self.unpack_ipv4) except exceptions.ValidationError: pass - return value + return six.text_type(value) def formfield(self, **kwargs): defaults = { diff --git a/docs/howto/custom-model-fields.txt b/docs/howto/custom-model-fields.txt index 12cc861746..614c63c999 100644 --- a/docs/howto/custom-model-fields.txt +++ b/docs/howto/custom-model-fields.txt @@ -593,6 +593,17 @@ For example:: return ''.join([''.join(l) for l in (value.north, value.east, value.south, value.west)]) +.. warning:: + + If your custom field uses the ``CHAR``, ``VARCHAR`` or ``TEXT`` + types for MySQL, you must make sure that :meth:`.get_prep_value` + always returns a string type. MySQL performs flexible and unexpected + matching when a query is performed on these types and the provided + value is an integer, which can cause queries to include unexpected + objects in their results. This problem cannot occur if you always + return a string type from :meth:`.get_prep_value`. + + Converting query values to database values ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/docs/ref/databases.txt b/docs/ref/databases.txt index 1ea497c1b4..a73c361876 100644 --- a/docs/ref/databases.txt +++ b/docs/ref/databases.txt @@ -507,6 +507,22 @@ MySQL does not support the ``NOWAIT`` option to the ``SELECT ... FOR UPDATE`` statement. If ``select_for_update()`` is used with ``nowait=True`` then a ``DatabaseError`` will be raised. +Automatic typecasting can cause unexpected results +-------------------------------------------------- + +When performing a query on a string type, but with an integer value, MySQL will +coerce the types of all values in the table to an integer before performing the +comparison. If your table contains the values ``'abc'``, ``'def'`` and you +query for ``WHERE mycolumn=0``, both rows will match. Similarly, ``WHERE mycolumn=1`` +will match the value ``'abc1'``. Therefore, string type fields included in Django +will always cast the value to a string before using it in a query. + +If you implement custom model fields that inherit from :class:`~django.db.models.Field` +directly, are overriding :meth:`~django.db.models.Field.get_prep_value`, or use +:meth:`extra() ` or +:meth:`raw() `, you should ensure that you +perform the appropriate typecasting. + .. _sqlite-notes: SQLite notes diff --git a/docs/ref/models/querysets.txt b/docs/ref/models/querysets.txt index bf18017405..91e757ea50 100644 --- a/docs/ref/models/querysets.txt +++ b/docs/ref/models/querysets.txt @@ -1189,6 +1189,16 @@ of the arguments is required, but you should use at least one of them. Entry.objects.extra(where=['headline=%s'], params=['Lennon']) +.. warning:: + + If you are performing queries on MySQL, note that MySQL's silent type coercion + may cause unexpected results when mixing types. If you query on a string + type column, but with an integer value, MySQL will coerce the types of all values + in the table to an integer before performing the comparison. For example, if your + table contains the values ``'abc'``, ``'def'`` and you query for ``WHERE mycolumn=0``, + both rows will match. To prevent this, perform the correct typecasting + before using the value in a query. + defer ~~~~~ diff --git a/docs/topics/db/sql.txt b/docs/topics/db/sql.txt index 2b2b4261f6..d8e1487c1a 100644 --- a/docs/topics/db/sql.txt +++ b/docs/topics/db/sql.txt @@ -66,6 +66,16 @@ options that make it very powerful. database, but does nothing to enforce that. If the query does not return rows, a (possibly cryptic) error will result. +.. warning:: + + If you are performing queries on MySQL, note that MySQL's silent type coercion + may cause unexpected results when mixing types. If you query on a string + type column, but with an integer value, MySQL will coerce the types of all values + in the table to an integer before performing the comparison. For example, if your + table contains the values ``'abc'``, ``'def'`` and you query for ``WHERE mycolumn=0``, + both rows will match. To prevent this, perform the correct typecasting + before using the value in a query. + Mapping query fields to model fields ------------------------------------ diff --git a/tests/model_fields/tests.py b/tests/model_fields/tests.py index e8c798adc5..c1c240cafb 100644 --- a/tests/model_fields/tests.py +++ b/tests/model_fields/tests.py @@ -673,12 +673,20 @@ class PromiseTest(test.TestCase): self.assertIsInstance( CharField().get_prep_value(lazy_func()), six.text_type) + lazy_func = lazy(lambda: 0, int) + self.assertIsInstance( + CharField().get_prep_value(lazy_func()), + six.text_type) def test_CommaSeparatedIntegerField(self): lazy_func = lazy(lambda: '1,2', six.text_type) self.assertIsInstance( CommaSeparatedIntegerField().get_prep_value(lazy_func()), six.text_type) + lazy_func = lazy(lambda: 0, int) + self.assertIsInstance( + CommaSeparatedIntegerField().get_prep_value(lazy_func()), + six.text_type) def test_DateField(self): lazy_func = lazy(lambda: datetime.date.today(), datetime.date) @@ -709,12 +717,20 @@ class PromiseTest(test.TestCase): self.assertIsInstance( FileField().get_prep_value(lazy_func()), six.text_type) + lazy_func = lazy(lambda: 0, int) + self.assertIsInstance( + FileField().get_prep_value(lazy_func()), + six.text_type) def test_FilePathField(self): lazy_func = lazy(lambda: 'tests.py', six.text_type) self.assertIsInstance( FilePathField().get_prep_value(lazy_func()), six.text_type) + lazy_func = lazy(lambda: 0, int) + self.assertIsInstance( + FilePathField().get_prep_value(lazy_func()), + six.text_type) def test_FloatField(self): lazy_func = lazy(lambda: 1.2, float) @@ -735,9 +751,13 @@ class PromiseTest(test.TestCase): int) def test_IPAddressField(self): - lazy_func = lazy(lambda: '127.0.0.1', six.text_type) with warnings.catch_warnings(record=True): warnings.simplefilter("always") + lazy_func = lazy(lambda: '127.0.0.1', six.text_type) + self.assertIsInstance( + IPAddressField().get_prep_value(lazy_func()), + six.text_type) + lazy_func = lazy(lambda: 0, int) self.assertIsInstance( IPAddressField().get_prep_value(lazy_func()), six.text_type) @@ -747,6 +767,10 @@ class PromiseTest(test.TestCase): self.assertIsInstance( GenericIPAddressField().get_prep_value(lazy_func()), six.text_type) + lazy_func = lazy(lambda: 0, int) + self.assertIsInstance( + GenericIPAddressField().get_prep_value(lazy_func()), + six.text_type) def test_NullBooleanField(self): lazy_func = lazy(lambda: True, bool) @@ -771,6 +795,10 @@ class PromiseTest(test.TestCase): self.assertIsInstance( SlugField().get_prep_value(lazy_func()), six.text_type) + lazy_func = lazy(lambda: 0, int) + self.assertIsInstance( + SlugField().get_prep_value(lazy_func()), + six.text_type) def test_SmallIntegerField(self): lazy_func = lazy(lambda: 1, int) @@ -783,6 +811,10 @@ class PromiseTest(test.TestCase): self.assertIsInstance( TextField().get_prep_value(lazy_func()), six.text_type) + lazy_func = lazy(lambda: 0, int) + self.assertIsInstance( + TextField().get_prep_value(lazy_func()), + six.text_type) def test_TimeField(self): lazy_func = lazy(lambda: datetime.datetime.now().time(), datetime.time)