From b55699968fc9ee985384c64e37f6cc74a0a23683 Mon Sep 17 00:00:00 2001 From: Mariusz Felisiak Date: Thu, 13 May 2021 08:53:44 +0200 Subject: [PATCH] Fixed #32718 -- Relaxed file name validation in FileField. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Validate filename returned by FileField.upload_to() not a filename passed to the FileField.generate_filename() (upload_to() may completely ignored passed filename). - Allow relative paths (without dot segments) in the generated filename. Thanks to Jakub Kleň for the report and review. Thanks to all folks for checking this patch on existing projects. Thanks Florian Apolloner and Markus Holtermann for the discussion and implementation idea. Regression in 0b79eb36915d178aef5c6a7bbce71b1e76d376d3. --- django/core/files/utils.py | 20 +++-- django/db/models/fields/files.py | 2 +- docs/releases/2.2.23.txt | 15 ++++ docs/releases/3.1.11.txt | 15 ++++ docs/releases/3.2.3.txt | 7 +- docs/releases/index.txt | 2 + tests/file_storage/test_generate_filename.py | 86 +++++++++++++++++--- tests/model_fields/test_filefield.py | 10 +++ 8 files changed, 140 insertions(+), 17 deletions(-) create mode 100644 docs/releases/2.2.23.txt create mode 100644 docs/releases/3.1.11.txt diff --git a/django/core/files/utils.py b/django/core/files/utils.py index f83cb1a3cf..f28cea1077 100644 --- a/django/core/files/utils.py +++ b/django/core/files/utils.py @@ -1,16 +1,26 @@ import os +import pathlib from django.core.exceptions import SuspiciousFileOperation -def validate_file_name(name): - if name != os.path.basename(name): - raise SuspiciousFileOperation("File name '%s' includes path elements" % name) - +def validate_file_name(name, allow_relative_path=False): # Remove potentially dangerous names - if name in {'', '.', '..'}: + if os.path.basename(name) in {'', '.', '..'}: raise SuspiciousFileOperation("Could not derive file name from '%s'" % name) + if allow_relative_path: + # Use PurePosixPath() because this branch is checked only in + # FileField.generate_filename() where all file paths are expected to be + # Unix style (with forward slashes). + path = pathlib.PurePosixPath(name) + if path.is_absolute() or '..' in path.parts: + raise SuspiciousFileOperation( + "Detected path traversal attempt in '%s'" % name + ) + elif name != os.path.basename(name): + raise SuspiciousFileOperation("File name '%s' includes path elements" % name) + return name diff --git a/django/db/models/fields/files.py b/django/db/models/fields/files.py index a2f972489f..18900f7b85 100644 --- a/django/db/models/fields/files.py +++ b/django/db/models/fields/files.py @@ -313,12 +313,12 @@ class FileField(Field): Until the storage layer, all file paths are expected to be Unix style (with forward slashes). """ - filename = validate_file_name(filename) if callable(self.upload_to): filename = self.upload_to(instance, filename) else: dirname = datetime.datetime.now().strftime(str(self.upload_to)) filename = posixpath.join(dirname, filename) + filename = validate_file_name(filename, allow_relative_path=True) return self.storage.generate_filename(filename) def save_form_data(self, instance, data): diff --git a/docs/releases/2.2.23.txt b/docs/releases/2.2.23.txt new file mode 100644 index 0000000000..6c39361e5f --- /dev/null +++ b/docs/releases/2.2.23.txt @@ -0,0 +1,15 @@ +=========================== +Django 2.2.23 release notes +=========================== + +*May 13, 2021* + +Django 2.2.23 fixes a regression in 2.2.21. + +Bugfixes +======== + +* Fixed a regression in Django 2.2.21 where saving ``FileField`` would raise a + ``SuspiciousFileOperation`` even when a custom + :attr:`~django.db.models.FileField.upload_to` returns a valid file path + (:ticket:`32718`). diff --git a/docs/releases/3.1.11.txt b/docs/releases/3.1.11.txt new file mode 100644 index 0000000000..d5fb537466 --- /dev/null +++ b/docs/releases/3.1.11.txt @@ -0,0 +1,15 @@ +=========================== +Django 3.1.11 release notes +=========================== + +*May 13, 2021* + +Django 3.1.11 fixes a regression in 3.1.9. + +Bugfixes +======== + +* Fixed a regression in Django 3.1.9 where saving ``FileField`` would raise a + ``SuspiciousFileOperation`` even when a custom + :attr:`~django.db.models.FileField.upload_to` returns a valid file path + (:ticket:`32718`). diff --git a/docs/releases/3.2.3.txt b/docs/releases/3.2.3.txt index 315678b92a..915590f85c 100644 --- a/docs/releases/3.2.3.txt +++ b/docs/releases/3.2.3.txt @@ -2,7 +2,7 @@ Django 3.2.3 release notes ========================== -*Expected June 1, 2021* +*May 13, 2021* Django 3.2.3 fixes several bugs in 3.2.2. @@ -13,3 +13,8 @@ Bugfixes * Fixed a regression in Django 3.2 that caused the incorrect filtering of querysets combined with the ``|`` operator (:ticket:`32717`). + +* Fixed a regression in Django 3.2.1 where saving ``FileField`` would raise a + ``SuspiciousFileOperation`` even when a custom + :attr:`~django.db.models.FileField.upload_to` returns a valid file path + (:ticket:`32718`). diff --git a/docs/releases/index.txt b/docs/releases/index.txt index a9a81767b2..e68df1ece1 100644 --- a/docs/releases/index.txt +++ b/docs/releases/index.txt @@ -42,6 +42,7 @@ versions of the documentation contain the release notes for any later releases. .. toctree:: :maxdepth: 1 + 3.1.11 3.1.10 3.1.9 3.1.8 @@ -80,6 +81,7 @@ versions of the documentation contain the release notes for any later releases. .. toctree:: :maxdepth: 1 + 2.2.23 2.2.22 2.2.21 2.2.20 diff --git a/tests/file_storage/test_generate_filename.py b/tests/file_storage/test_generate_filename.py index 4746a53f69..66551c495b 100644 --- a/tests/file_storage/test_generate_filename.py +++ b/tests/file_storage/test_generate_filename.py @@ -1,6 +1,4 @@ import os -import sys -from unittest import skipIf from django.core.exceptions import SuspiciousFileOperation from django.core.files.base import ContentFile @@ -64,19 +62,37 @@ class GenerateFilenameStorageTests(SimpleTestCase): s.generate_filename(file_name) def test_filefield_dangerous_filename(self): - candidates = ['..', '.', '', '???', '$.$.$'] + candidates = [ + ('..', 'some/folder/..'), + ('.', 'some/folder/.'), + ('', 'some/folder/'), + ('???', '???'), + ('$.$.$', '$.$.$'), + ] f = FileField(upload_to='some/folder/') - msg = "Could not derive file name from '%s'" - for file_name in candidates: + for file_name, msg_file_name in candidates: + msg = f"Could not derive file name from '{msg_file_name}'" with self.subTest(file_name=file_name): - with self.assertRaisesMessage(SuspiciousFileOperation, msg % file_name): + with self.assertRaisesMessage(SuspiciousFileOperation, msg): f.generate_filename(None, file_name) - def test_filefield_dangerous_filename_dir(self): + def test_filefield_dangerous_filename_dot_segments(self): f = FileField(upload_to='some/folder/') - msg = "File name '/tmp/path' includes path elements" + msg = "Detected path traversal attempt in 'some/folder/../path'" with self.assertRaisesMessage(SuspiciousFileOperation, msg): - f.generate_filename(None, '/tmp/path') + f.generate_filename(None, '../path') + + def test_filefield_generate_filename_absolute_path(self): + f = FileField(upload_to='some/folder/') + candidates = [ + '/tmp/path', + '/tmp/../path', + ] + for file_name in candidates: + msg = f"Detected path traversal attempt in '{file_name}'" + with self.subTest(file_name=file_name): + with self.assertRaisesMessage(SuspiciousFileOperation, msg): + f.generate_filename(None, file_name) def test_filefield_generate_filename(self): f = FileField(upload_to='some/folder/') @@ -95,7 +111,57 @@ class GenerateFilenameStorageTests(SimpleTestCase): os.path.normpath('some/folder/test_with_space.txt') ) - @skipIf(sys.platform == 'win32', 'Path components in filename are not supported after 0b79eb3.') + def test_filefield_generate_filename_upload_to_overrides_dangerous_filename(self): + def upload_to(instance, filename): + return 'test.txt' + + f = FileField(upload_to=upload_to) + candidates = [ + '/tmp/.', + '/tmp/..', + '/tmp/../path', + '/tmp/path', + 'some/folder/', + 'some/folder/.', + 'some/folder/..', + 'some/folder/???', + 'some/folder/$.$.$', + 'some/../test.txt', + '', + ] + for file_name in candidates: + with self.subTest(file_name=file_name): + self.assertEqual(f.generate_filename(None, file_name), 'test.txt') + + def test_filefield_generate_filename_upload_to_absolute_path(self): + def upload_to(instance, filename): + return '/tmp/' + filename + + f = FileField(upload_to=upload_to) + candidates = [ + 'path', + '../path', + '???', + '$.$.$', + ] + for file_name in candidates: + msg = f"Detected path traversal attempt in '/tmp/{file_name}'" + with self.subTest(file_name=file_name): + with self.assertRaisesMessage(SuspiciousFileOperation, msg): + f.generate_filename(None, file_name) + + def test_filefield_generate_filename_upload_to_dangerous_filename(self): + def upload_to(instance, filename): + return '/tmp/' + filename + + f = FileField(upload_to=upload_to) + candidates = ['..', '.', ''] + for file_name in candidates: + msg = f"Could not derive file name from '/tmp/{file_name}'" + with self.subTest(file_name=file_name): + with self.assertRaisesMessage(SuspiciousFileOperation, msg): + f.generate_filename(None, file_name) + def test_filefield_awss3_storage(self): """ Simulate a FileField with an S3 storage which uses keys rather than diff --git a/tests/model_fields/test_filefield.py b/tests/model_fields/test_filefield.py index 51e29f6d25..fb9426e2d1 100644 --- a/tests/model_fields/test_filefield.py +++ b/tests/model_fields/test_filefield.py @@ -5,6 +5,7 @@ import tempfile import unittest from pathlib import Path +from django.core.exceptions import SuspiciousFileOperation from django.core.files import File, temp from django.core.files.base import ContentFile from django.core.files.uploadedfile import TemporaryUploadedFile @@ -63,6 +64,15 @@ class FileFieldTests(TestCase): d.refresh_from_db() self.assertIs(d.myfile.instance, d) + @unittest.skipIf(sys.platform == 'win32', "Crashes with OSError on Windows.") + def test_save_without_name(self): + with tempfile.NamedTemporaryFile(suffix='.txt') as tmp: + document = Document.objects.create(myfile='something.txt') + document.myfile = File(tmp) + msg = f"Detected path traversal attempt in '{tmp.name}'" + with self.assertRaisesMessage(SuspiciousFileOperation, msg): + document.save() + def test_defer(self): Document.objects.create(myfile='something.txt') self.assertEqual(Document.objects.defer('myfile')[0].myfile, 'something.txt')