From d7a06ee7e571b6dad07c0f5b519b1db02e2a476c Mon Sep 17 00:00:00 2001
From: Tim Graham
Date: Thu, 11 Dec 2014 08:31:03 -0500
Subject: [PATCH] [1.6.x] Fixed DoS possibility in ModelMultipleChoiceField.
This is a security fix. Disclosure following shortly.
Thanks Keryn Knight for the report and initial patch.
---
django/forms/models.py | 28 +++++++++++++++++++++++-----
docs/releases/1.6.10.txt | 9 +++++++++
tests/model_forms/tests.py | 21 +++++++++++++++++++++
3 files changed, 53 insertions(+), 5 deletions(-)
diff --git a/django/forms/models.py b/django/forms/models.py
index bf28d95414..4b1afe41c2 100644
--- a/django/forms/models.py
+++ b/django/forms/models.py
@@ -1174,8 +1174,7 @@ class ModelMultipleChoiceField(ModelChoiceField):
def to_python(self, value):
if not value:
return []
- to_py = super(ModelMultipleChoiceField, self).to_python
- return [to_py(val) for val in value]
+ return list(self._check_values(value))
def clean(self, value):
if self.required and not value:
@@ -1184,7 +1183,29 @@ class ModelMultipleChoiceField(ModelChoiceField):
return self.queryset.none()
if not isinstance(value, (list, tuple)):
raise ValidationError(self.error_messages['list'], code='list')
+ qs = self._check_values(value)
+ # Since this overrides the inherited ModelChoiceField.clean
+ # we run custom validators here
+ self.run_validators(value)
+ return qs
+
+ def _check_values(self, value):
+ """
+ Given a list of possible PK values, returns a QuerySet of the
+ corresponding objects. Raises a ValidationError if a given value is
+ invalid (not a valid PK, not in the queryset, etc.)
+ """
key = self.to_field_name or 'pk'
+ # deduplicate given values to avoid creating many querysets or
+ # requiring the database backend deduplicate efficiently.
+ try:
+ value = frozenset(value)
+ except TypeError:
+ # list of lists isn't hashable, for example
+ raise ValidationError(
+ self.error_messages['list'],
+ code='list',
+ )
for pk in value:
try:
self.queryset.filter(**{key: pk})
@@ -1203,9 +1224,6 @@ class ModelMultipleChoiceField(ModelChoiceField):
code='invalid_choice',
params={'value': val},
)
- # Since this overrides the inherited ModelChoiceField.clean
- # we run custom validators here
- self.run_validators(value)
return qs
def prepare_value(self, value):
diff --git a/docs/releases/1.6.10.txt b/docs/releases/1.6.10.txt
index 20aa595b77..5b8f0cdec3 100644
--- a/docs/releases/1.6.10.txt
+++ b/docs/releases/1.6.10.txt
@@ -58,3 +58,12 @@ Note, however, that this view has always carried a warning that it is not
hardened for production use and should be used only as a development aid. Now
may be a good time to audit your project and serve your files in production
using a real front-end web server if you are not doing so.
+
+Database denial-of-service with ``ModelMultipleChoiceField``
+============================================================
+
+Given a form that uses ``ModelMultipleChoiceField`` and
+``show_hidden_initial=True`` (not a documented API), it was possible for a user
+to cause an unreasonable number of SQL queries by submitting duplicate values
+for the field's data. The validation logic in ``ModelMultipleChoiceField`` now
+deduplicates submitted values to address this issue.
diff --git a/tests/model_forms/tests.py b/tests/model_forms/tests.py
index e33372f0ba..4839e1f8f3 100644
--- a/tests/model_forms/tests.py
+++ b/tests/model_forms/tests.py
@@ -1381,6 +1381,27 @@ class OldFormForXTests(TestCase):
''' % (w_woodward.pk, w_bernstein.pk, bw.pk, w_royko.pk))
+ def test_show_hidden_initial_changed_queries_efficiently(self):
+ class WriterForm(forms.Form):
+ persons = forms.ModelMultipleChoiceField(
+ show_hidden_initial=True, queryset=Writer.objects.all())
+
+ writers = (Writer.objects.create(name=str(x)) for x in range(0, 50))
+ writer_pks = tuple(x.pk for x in writers)
+ form = WriterForm(data={'initial-persons': writer_pks})
+ with self.assertNumQueries(1):
+ self.assertTrue(form.has_changed())
+
+ def test_clean_does_deduplicate_values(self):
+ class WriterForm(forms.Form):
+ persons = forms.ModelMultipleChoiceField(queryset=Writer.objects.all())
+
+ person1 = Writer.objects.create(name="Person 1")
+ form = WriterForm(data={})
+ queryset = form.fields['persons'].clean([str(person1.pk)] * 50)
+ sql, params = queryset.query.sql_with_params()
+ self.assertEqual(len(params), 1)
+
def test_file_field(self):
# Test conditions when files is either not given or empty.