From fb4c55d9ec4bb812a7fb91fa20510d91645e411b Mon Sep 17 00:00:00 2001 From: Mariusz Felisiak Date: Thu, 13 Apr 2023 10:10:56 +0200 Subject: [PATCH] Fixed CVE-2023-31047, Fixed #31710 -- Prevented potential bypass of validation when uploading multiple files using one form field. Thanks Moataz Al-Sharida and nawaik for reports. Co-authored-by: Shai Berger Co-authored-by: nessita <124304+nessita@users.noreply.github.com> --- django/forms/widgets.py | 26 ++++++- docs/releases/3.2.19.txt | 16 ++++- docs/releases/4.1.9.txt | 16 ++++- docs/releases/4.2.1.txt | 16 +++++ docs/topics/http/file-uploads.txt | 66 +++++++++++++++--- .../forms_tests/field_tests/test_filefield.py | 68 ++++++++++++++++++- .../widget_tests/test_clearablefileinput.py | 5 ++ .../widget_tests/test_fileinput.py | 44 ++++++++++++ 8 files changed, 245 insertions(+), 12 deletions(-) diff --git a/django/forms/widgets.py b/django/forms/widgets.py index b4a9583364..ab7c0f755f 100644 --- a/django/forms/widgets.py +++ b/django/forms/widgets.py @@ -406,17 +406,41 @@ class MultipleHiddenInput(HiddenInput): class FileInput(Input): + allow_multiple_selected = False input_type = "file" needs_multipart_form = True template_name = "django/forms/widgets/file.html" + def __init__(self, attrs=None): + if ( + attrs is not None + and not self.allow_multiple_selected + and attrs.get("multiple", False) + ): + raise ValueError( + "%s doesn't support uploading multiple files." + % self.__class__.__qualname__ + ) + if self.allow_multiple_selected: + if attrs is None: + attrs = {"multiple": True} + else: + attrs.setdefault("multiple", True) + super().__init__(attrs) + def format_value(self, value): """File input never renders a value.""" return def value_from_datadict(self, data, files, name): "File widgets take data from FILES, not POST" - return files.get(name) + getter = files.get + if self.allow_multiple_selected: + try: + getter = files.getlist + except AttributeError: + pass + return getter(name) def value_omitted_from_data(self, data, files, name): return name not in files diff --git a/docs/releases/3.2.19.txt b/docs/releases/3.2.19.txt index c5817e689c..9f9eb3f45c 100644 --- a/docs/releases/3.2.19.txt +++ b/docs/releases/3.2.19.txt @@ -6,4 +6,18 @@ Django 3.2.19 release notes Django 3.2.19 fixes a security issue with severity "low" in 3.2.18. -... +CVE-2023-31047: Potential bypass of validation when uploading multiple files using one form field +================================================================================================= + +Uploading multiple files using one form field has never been supported by +:class:`.forms.FileField` or :class:`.forms.ImageField` as only the last +uploaded file was validated. Unfortunately, :ref:`uploading_multiple_files` +topic suggested otherwise. + +In order to avoid the vulnerability, :class:`~django.forms.ClearableFileInput` +and :class:`~django.forms.FileInput` form widgets now raise ``ValueError`` when +the ``multiple`` HTML attribute is set on them. To prevent the exception and +keep the old behavior, set ``allow_multiple_selected`` to ``True``. + +For more details on using the new attribute and handling of multiple files +through a single field, see :ref:`uploading_multiple_files`. diff --git a/docs/releases/4.1.9.txt b/docs/releases/4.1.9.txt index 87b6690685..77c2b3547b 100644 --- a/docs/releases/4.1.9.txt +++ b/docs/releases/4.1.9.txt @@ -6,4 +6,18 @@ Django 4.1.9 release notes Django 4.1.9 fixes a security issue with severity "low" in 4.1.8. -... +CVE-2023-31047: Potential bypass of validation when uploading multiple files using one form field +================================================================================================= + +Uploading multiple files using one form field has never been supported by +:class:`.forms.FileField` or :class:`.forms.ImageField` as only the last +uploaded file was validated. Unfortunately, :ref:`uploading_multiple_files` +topic suggested otherwise. + +In order to avoid the vulnerability, :class:`~django.forms.ClearableFileInput` +and :class:`~django.forms.FileInput` form widgets now raise ``ValueError`` when +the ``multiple`` HTML attribute is set on them. To prevent the exception and +keep the old behavior, set ``allow_multiple_selected`` to ``True``. + +For more details on using the new attribute and handling of multiple files +through a single field, see :ref:`uploading_multiple_files`. diff --git a/docs/releases/4.2.1.txt b/docs/releases/4.2.1.txt index bed64f6ad1..7977e7f088 100644 --- a/docs/releases/4.2.1.txt +++ b/docs/releases/4.2.1.txt @@ -7,6 +7,22 @@ Django 4.2.1 release notes Django 4.2.1 fixes a security issue with severity "low" and several bugs in 4.2. +CVE-2023-31047: Potential bypass of validation when uploading multiple files using one form field +================================================================================================= + +Uploading multiple files using one form field has never been supported by +:class:`.forms.FileField` or :class:`.forms.ImageField` as only the last +uploaded file was validated. Unfortunately, :ref:`uploading_multiple_files` +topic suggested otherwise. + +In order to avoid the vulnerability, :class:`~django.forms.ClearableFileInput` +and :class:`~django.forms.FileInput` form widgets now raise ``ValueError`` when +the ``multiple`` HTML attribute is set on them. To prevent the exception and +keep the old behavior, set ``allow_multiple_selected`` to ``True``. + +For more details on using the new attribute and handling of multiple files +through a single field, see :ref:`uploading_multiple_files`. + Bugfixes ======== diff --git a/docs/topics/http/file-uploads.txt b/docs/topics/http/file-uploads.txt index 7bd071d6c9..cb0fb5b7fb 100644 --- a/docs/topics/http/file-uploads.txt +++ b/docs/topics/http/file-uploads.txt @@ -144,11 +144,27 @@ a :class:`~django.core.files.File` like object to the instance = ModelWithFileField(file_field=content_file) instance.save() +.. _uploading_multiple_files: + Uploading multiple files ------------------------ -If you want to upload multiple files using one form field, set the ``multiple`` -HTML attribute of field's widget: +.. + Tests in tests.forms_tests.field_tests.test_filefield.MultipleFileFieldTest + should be updated after any changes in the following snippets. + +If you want to upload multiple files using one form field, create a subclass +of the field's widget and set the ``allow_multiple_selected`` attribute on it +to ``True``. + +In order for such files to be all validated by your form (and have the value of +the field include them all), you will also have to subclass ``FileField``. See +below for an example. + +.. admonition:: Multiple file field + + Django is likely to have a proper multiple file field support at some point + in the future. .. code-block:: python :caption: ``forms.py`` @@ -156,10 +172,26 @@ HTML attribute of field's widget: from django import forms + class MultipleFileInput(forms.ClearableFileInput): + allow_multiple_selected = True + + + class MultipleFileField(forms.FileField): + def __init__(self, *args, **kwargs): + kwargs.setdefault("widget", MultipleFileInput()) + super().__init__(*args, **kwargs) + + def clean(self, data, initial=None): + single_file_clean = super().clean + if isinstance(data, (list, tuple)): + result = [single_file_clean(d, initial) for d in data] + else: + result = single_file_clean(data, initial) + return result + + class FileFieldForm(forms.Form): - file_field = forms.FileField( - widget=forms.ClearableFileInput(attrs={"multiple": True}) - ) + file_field = MultipleFileField() Then override the ``post`` method of your :class:`~django.views.generic.edit.FormView` subclass to handle multiple file @@ -180,14 +212,32 @@ uploads: def post(self, request, *args, **kwargs): form_class = self.get_form_class() form = self.get_form(form_class) - files = request.FILES.getlist("file_field") if form.is_valid(): - for f in files: - ... # Do something with each file. return self.form_valid(form) else: return self.form_invalid(form) + def form_valid(self, form): + files = form.cleaned_data["file_field"] + for f in files: + ... # Do something with each file. + return super().form_valid() + +.. warning:: + + This will allow you to handle multiple files at the form level only. Be + aware that you cannot use it to put multiple files on a single model + instance (in a single field), for example, even if the custom widget is used + with a form field related to a model ``FileField``. + +.. versionchanged:: 3.2.19 + + In previous versions, there was no support for the ``allow_multiple_selected`` + class attribute, and users were advised to create the widget with the HTML + attribute ``multiple`` set through the ``attrs`` argument. However, this + caused validation of the form field to be applied only to the last file + submitted, which could have adverse security implications. + Upload Handlers =============== diff --git a/tests/forms_tests/field_tests/test_filefield.py b/tests/forms_tests/field_tests/test_filefield.py index 56aaa311e0..00c74a7c1a 100644 --- a/tests/forms_tests/field_tests/test_filefield.py +++ b/tests/forms_tests/field_tests/test_filefield.py @@ -2,7 +2,8 @@ import pickle from django.core.exceptions import ValidationError from django.core.files.uploadedfile import SimpleUploadedFile -from django.forms import FileField +from django.core.validators import validate_image_file_extension +from django.forms import FileField, FileInput from django.test import SimpleTestCase @@ -109,3 +110,68 @@ class FileFieldTest(SimpleTestCase): def test_file_picklable(self): self.assertIsInstance(pickle.loads(pickle.dumps(FileField())), FileField) + + +class MultipleFileInput(FileInput): + allow_multiple_selected = True + + +class MultipleFileField(FileField): + def __init__(self, *args, **kwargs): + kwargs.setdefault("widget", MultipleFileInput()) + super().__init__(*args, **kwargs) + + def clean(self, data, initial=None): + single_file_clean = super().clean + if isinstance(data, (list, tuple)): + result = [single_file_clean(d, initial) for d in data] + else: + result = single_file_clean(data, initial) + return result + + +class MultipleFileFieldTest(SimpleTestCase): + def test_file_multiple(self): + f = MultipleFileField() + files = [ + SimpleUploadedFile("name1", b"Content 1"), + SimpleUploadedFile("name2", b"Content 2"), + ] + self.assertEqual(f.clean(files), files) + + def test_file_multiple_empty(self): + f = MultipleFileField() + files = [ + SimpleUploadedFile("empty", b""), + SimpleUploadedFile("nonempty", b"Some Content"), + ] + msg = "'The submitted file is empty.'" + with self.assertRaisesMessage(ValidationError, msg): + f.clean(files) + with self.assertRaisesMessage(ValidationError, msg): + f.clean(files[::-1]) + + def test_file_multiple_validation(self): + f = MultipleFileField(validators=[validate_image_file_extension]) + + good_files = [ + SimpleUploadedFile("image1.jpg", b"fake JPEG"), + SimpleUploadedFile("image2.png", b"faux image"), + SimpleUploadedFile("image3.bmp", b"fraudulent bitmap"), + ] + self.assertEqual(f.clean(good_files), good_files) + + evil_files = [ + SimpleUploadedFile("image1.sh", b"#!/bin/bash -c 'echo pwned!'\n"), + SimpleUploadedFile("image2.png", b"faux image"), + SimpleUploadedFile("image3.jpg", b"fake JPEG"), + ] + + evil_rotations = ( + evil_files[i:] + evil_files[:i] # Rotate by i. + for i in range(len(evil_files)) + ) + msg = "File extension “sh” is not allowed. Allowed extensions are: " + for rotated_evil_files in evil_rotations: + with self.assertRaisesMessage(ValidationError, msg): + f.clean(rotated_evil_files) diff --git a/tests/forms_tests/widget_tests/test_clearablefileinput.py b/tests/forms_tests/widget_tests/test_clearablefileinput.py index 8d5f0c5f98..53b84cc0ec 100644 --- a/tests/forms_tests/widget_tests/test_clearablefileinput.py +++ b/tests/forms_tests/widget_tests/test_clearablefileinput.py @@ -245,3 +245,8 @@ class ClearableFileInputTest(WidgetTest): '', form.render(), ) + + def test_multiple_error(self): + msg = "ClearableFileInput doesn't support uploading multiple files." + with self.assertRaisesMessage(ValueError, msg): + ClearableFileInput(attrs={"multiple": True}) diff --git a/tests/forms_tests/widget_tests/test_fileinput.py b/tests/forms_tests/widget_tests/test_fileinput.py index ea73e577ce..a49f481728 100644 --- a/tests/forms_tests/widget_tests/test_fileinput.py +++ b/tests/forms_tests/widget_tests/test_fileinput.py @@ -1,4 +1,6 @@ +from django.core.files.uploadedfile import SimpleUploadedFile from django.forms import FileField, FileInput, Form +from django.utils.datastructures import MultiValueDict from .base import WidgetTest @@ -48,3 +50,45 @@ class FileInputTest(WidgetTest): 'name="field" required type="file">', form.render(), ) + + def test_multiple_error(self): + msg = "FileInput doesn't support uploading multiple files." + with self.assertRaisesMessage(ValueError, msg): + FileInput(attrs={"multiple": True}) + + def test_value_from_datadict_multiple(self): + class MultipleFileInput(FileInput): + allow_multiple_selected = True + + file_1 = SimpleUploadedFile("something1.txt", b"content 1") + file_2 = SimpleUploadedFile("something2.txt", b"content 2") + # Uploading multiple files is allowed. + widget = MultipleFileInput(attrs={"multiple": True}) + value = widget.value_from_datadict( + data={"name": "Test name"}, + files=MultiValueDict({"myfile": [file_1, file_2]}), + name="myfile", + ) + self.assertEqual(value, [file_1, file_2]) + # Uploading multiple files is not allowed. + widget = FileInput() + value = widget.value_from_datadict( + data={"name": "Test name"}, + files=MultiValueDict({"myfile": [file_1, file_2]}), + name="myfile", + ) + self.assertEqual(value, file_2) + + def test_multiple_default(self): + class MultipleFileInput(FileInput): + allow_multiple_selected = True + + tests = [ + (None, True), + ({"class": "myclass"}, True), + ({"multiple": False}, False), + ] + for attrs, expected in tests: + with self.subTest(attrs=attrs): + widget = MultipleFileInput(attrs=attrs) + self.assertIs(widget.attrs["multiple"], expected)