From 35c991aa06aa34fa458f01eac49275ff4c2d76f9 Mon Sep 17 00:00:00 2001 From: Aymeric Augustin Date: Tue, 12 Feb 2013 11:22:41 +0100 Subject: [PATCH] Added a default limit to the maximum number of forms in a formset. This is a security fix. Disclosure and advisory coming shortly. --- django/forms/formsets.py | 23 +++--- docs/topics/forms/formsets.txt | 4 +- docs/topics/forms/modelforms.txt | 4 +- tests/regressiontests/forms/tests/formsets.py | 70 +++++++++++++++++-- .../generic_inline_admin/tests.py | 3 +- 5 files changed, 85 insertions(+), 19 deletions(-) diff --git a/django/forms/formsets.py b/django/forms/formsets.py index 1addbc617b..81b75f2796 100644 --- a/django/forms/formsets.py +++ b/django/forms/formsets.py @@ -21,6 +21,9 @@ MAX_NUM_FORM_COUNT = 'MAX_NUM_FORMS' ORDERING_FIELD_NAME = 'ORDER' DELETION_FIELD_NAME = 'DELETE' +# default maximum number of forms in a formset, to prevent memory exhaustion +DEFAULT_MAX_NUM = 1000 + class ManagementForm(Form): """ ``ManagementForm`` is used to keep track of how many form instances @@ -97,11 +100,10 @@ class BaseFormSet(object): total_forms = initial_forms + self.extra # Allow all existing related objects/inlines to be displayed, # but don't allow extra beyond max_num. - if self.max_num is not None: - if initial_forms > self.max_num >= 0: - total_forms = initial_forms - elif total_forms > self.max_num >= 0: - total_forms = self.max_num + if initial_forms > self.max_num >= 0: + total_forms = initial_forms + elif total_forms > self.max_num >= 0: + total_forms = self.max_num return total_forms def initial_form_count(self): @@ -111,14 +113,14 @@ class BaseFormSet(object): else: # Use the length of the inital data if it's there, 0 otherwise. initial_forms = self.initial and len(self.initial) or 0 - if self.max_num is not None and (initial_forms > self.max_num >= 0): + if initial_forms > self.max_num >= 0: initial_forms = self.max_num return initial_forms def _construct_forms(self): # instantiate all the forms and put them in self.forms self.forms = [] - for i in xrange(self.total_form_count()): + for i in xrange(min(self.total_form_count(), self.absolute_max)): self.forms.append(self._construct_form(i)) def _construct_form(self, i, **kwargs): @@ -367,9 +369,14 @@ class BaseFormSet(object): def formset_factory(form, formset=BaseFormSet, extra=1, can_order=False, can_delete=False, max_num=None): """Return a FormSet for the given form class.""" + if max_num is None: + max_num = DEFAULT_MAX_NUM + # hard limit on forms instantiated, to prevent memory-exhaustion attacks + # limit defaults to DEFAULT_MAX_NUM, but developer can increase it via max_num + absolute_max = max(DEFAULT_MAX_NUM, max_num) attrs = {'form': form, 'extra': extra, 'can_order': can_order, 'can_delete': can_delete, - 'max_num': max_num} + 'max_num': max_num, 'absolute_max': absolute_max} return type(form.__name__ + str('FormSet'), (formset,), attrs) def all_valid(formsets): diff --git a/docs/topics/forms/formsets.txt b/docs/topics/forms/formsets.txt index e2a2b00c7d..d2d102b5d6 100644 --- a/docs/topics/forms/formsets.txt +++ b/docs/topics/forms/formsets.txt @@ -98,8 +98,8 @@ If the value of ``max_num`` is greater than the number of existing objects, up to ``extra`` additional blank forms will be added to the formset, so long as the total number of forms does not exceed ``max_num``. -A ``max_num`` value of ``None`` (the default) puts no limit on the number of -forms displayed. +A ``max_num`` value of ``None`` (the default) puts a high limit on the number +of forms displayed (1000). In practice this is equivalent to no limit. Formset validation ------------------ diff --git a/docs/topics/forms/modelforms.txt b/docs/topics/forms/modelforms.txt index fec0d14836..62020e461e 100644 --- a/docs/topics/forms/modelforms.txt +++ b/docs/topics/forms/modelforms.txt @@ -738,8 +738,8 @@ so long as the total number of forms does not exceed ``max_num``:: -A ``max_num`` value of ``None`` (the default) puts no limit on the number of -forms displayed. +A ``max_num`` value of ``None`` (the default) puts a high limit on the number +of forms displayed (1000). In practice this is equivalent to no limit. Using a model formset in a view ------------------------------- diff --git a/tests/regressiontests/forms/tests/formsets.py b/tests/regressiontests/forms/tests/formsets.py index ef6f40c3e3..573a8f6a6d 100644 --- a/tests/regressiontests/forms/tests/formsets.py +++ b/tests/regressiontests/forms/tests/formsets.py @@ -2,7 +2,7 @@ from __future__ import unicode_literals from django.forms import (CharField, DateField, FileField, Form, IntegerField, - ValidationError) + ValidationError, formsets) from django.forms.formsets import BaseFormSet, formset_factory from django.forms.util import ErrorList from django.test import TestCase @@ -51,7 +51,7 @@ class FormsFormsetTestCase(TestCase): # for adding data. By default, it displays 1 blank form. It can display more, # but we'll look at how to do so later. formset = ChoiceFormSet(auto_id=False, prefix='choices') - self.assertHTMLEqual(str(formset), """ + self.assertHTMLEqual(str(formset), """ Choice: Votes:""") @@ -654,8 +654,8 @@ class FormsFormsetTestCase(TestCase): # Limiting the maximum number of forms ######################################## # Base case for max_num. - # When not passed, max_num will take its default value of None, i.e. unlimited - # number of forms, only controlled by the value of the extra parameter. + # When not passed, max_num will take a high default value, leaving the + # number of forms only controlled by the value of the extra parameter. LimitedFavoriteDrinkFormSet = formset_factory(FavoriteDrinkForm, extra=3) formset = LimitedFavoriteDrinkFormSet() @@ -702,8 +702,8 @@ class FormsFormsetTestCase(TestCase): def test_max_num_with_initial_data(self): # max_num with initial data - # When not passed, max_num will take its default value of None, i.e. unlimited - # number of forms, only controlled by the values of the initial and extra + # When not passed, max_num will take a high default value, leaving the + # number of forms only controlled by the value of the initial and extra # parameters. initial = [ @@ -878,6 +878,64 @@ class FormsFormsetTestCase(TestCase): self.assertTrue(formset.is_valid()) self.assertTrue(all([form.is_valid_called for form in formset.forms])) + def test_hard_limit_on_instantiated_forms(self): + """A formset has a hard limit on the number of forms instantiated.""" + # reduce the default limit of 1000 temporarily for testing + _old_DEFAULT_MAX_NUM = formsets.DEFAULT_MAX_NUM + try: + formsets.DEFAULT_MAX_NUM = 3 + ChoiceFormSet = formset_factory(Choice) + # someone fiddles with the mgmt form data... + formset = ChoiceFormSet( + { + 'choices-TOTAL_FORMS': '4', + 'choices-INITIAL_FORMS': '0', + 'choices-MAX_NUM_FORMS': '4', + 'choices-0-choice': 'Zero', + 'choices-0-votes': '0', + 'choices-1-choice': 'One', + 'choices-1-votes': '1', + 'choices-2-choice': 'Two', + 'choices-2-votes': '2', + 'choices-3-choice': 'Three', + 'choices-3-votes': '3', + }, + prefix='choices', + ) + # But we still only instantiate 3 forms + self.assertEqual(len(formset.forms), 3) + finally: + formsets.DEFAULT_MAX_NUM = _old_DEFAULT_MAX_NUM + + def test_increase_hard_limit(self): + """Can increase the built-in forms limit via a higher max_num.""" + # reduce the default limit of 1000 temporarily for testing + _old_DEFAULT_MAX_NUM = formsets.DEFAULT_MAX_NUM + try: + formsets.DEFAULT_MAX_NUM = 3 + # for this form, we want a limit of 4 + ChoiceFormSet = formset_factory(Choice, max_num=4) + formset = ChoiceFormSet( + { + 'choices-TOTAL_FORMS': '4', + 'choices-INITIAL_FORMS': '0', + 'choices-MAX_NUM_FORMS': '4', + 'choices-0-choice': 'Zero', + 'choices-0-votes': '0', + 'choices-1-choice': 'One', + 'choices-1-votes': '1', + 'choices-2-choice': 'Two', + 'choices-2-votes': '2', + 'choices-3-choice': 'Three', + 'choices-3-votes': '3', + }, + prefix='choices', + ) + # This time four forms are instantiated + self.assertEqual(len(formset.forms), 4) + finally: + formsets.DEFAULT_MAX_NUM = _old_DEFAULT_MAX_NUM + data = { 'choices-TOTAL_FORMS': '1', # the number of forms rendered diff --git a/tests/regressiontests/generic_inline_admin/tests.py b/tests/regressiontests/generic_inline_admin/tests.py index f03641d292..8ba1700c76 100644 --- a/tests/regressiontests/generic_inline_admin/tests.py +++ b/tests/regressiontests/generic_inline_admin/tests.py @@ -6,6 +6,7 @@ from django.contrib import admin from django.contrib.admin.sites import AdminSite from django.contrib.contenttypes.generic import ( generic_inlineformset_factory, GenericTabularInline) +from django.forms.formsets import DEFAULT_MAX_NUM from django.forms.models import ModelForm from django.test import TestCase from django.test.utils import override_settings @@ -244,7 +245,7 @@ class GenericInlineModelAdminTest(TestCase): # Create a formset with default arguments formset = media_inline.get_formset(request) - self.assertEqual(formset.max_num, None) + self.assertEqual(formset.max_num, DEFAULT_MAX_NUM) self.assertEqual(formset.can_order, False) # Create a formset with custom keyword arguments