Skip to content
Permalink
Browse files

[1.7.x] Fixed DoS possibility in ModelMultipleChoiceField.

This is a security fix. Disclosure following shortly.

Thanks Keryn Knight for the report and initial patch.
  • Loading branch information...
timgraham committed Dec 11, 2014
1 parent 818e59a commit bcfb47780ce7caecb409a9e9c1c314266e41d392
Showing with 63 additions and 5 deletions.
  1. +23 −5 django/forms/models.py
  2. +9 −0 docs/releases/1.6.10.txt
  3. +9 −0 docs/releases/1.7.3.txt
  4. +1 −0 docs/spelling_wordlist
  5. +21 −0 tests/model_forms/tests.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):
@@ -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.
@@ -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
========

@@ -134,6 +134,7 @@ dbshell
de
deconstruct
deconstructing
deduplicates
deepcopy
deserialization
deserialize
@@ -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):

0 comments on commit bcfb477

Please sign in to comment.
You can’t perform that action at this time.