[1.7.x] Fixed DoS possibility in ModelMultipleChoiceField.

timgraham · timgraham · commit bcfb47780ce7 · 2015-01-13T13:02:56.000-05:00
This is a security fix. Disclosure following shortly.

Thanks Keryn Knight for the report and initial patch.
diff --git a/django/forms/models.py b/django/forms/models.py
@@ -1221,8 +1221,7 @@ def __init__(self, queryset, cache_choices=False, required=True,
     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:
@@ -1231,7 +1230,29 @@ def clean(self, value):
             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})
@@ -1250,9 +1271,6 @@ def clean(self, value):
                     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
@@ -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/docs/releases/1.7.3.txt b/docs/releases/1.7.3.txt
@@ -59,6 +59,15 @@ 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.
+
 Bugfixes
 ========
 
diff --git a/docs/spelling_wordlist b/docs/spelling_wordlist
@@ -134,6 +134,7 @@ dbshell
 de
 deconstruct
 deconstructing
+deduplicates
 deepcopy
 deserialization
 deserialize
diff --git a/tests/model_forms/tests.py b/tests/model_forms/tests.py
@@ -1573,6 +1573,27 @@ class WriterForm(forms.Form):
         self.assertTrue(form.is_valid())
         self.assertTrue(form.has_changed())
 
+    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)
+
 
 class ModelOneToOneFieldTests(TestCase):
     def test_modelform_onetoonefield(self):
