diff options
| -rw-r--r-- | django/core/files/utils.py | 20 | ||||
| -rw-r--r-- | django/db/models/fields/files.py | 2 | ||||
| -rw-r--r-- | docs/releases/2.2.23.txt | 15 | ||||
| -rw-r--r-- | docs/releases/3.1.11.txt | 15 | ||||
| -rw-r--r-- | docs/releases/index.txt | 2 | ||||
| -rw-r--r-- | tests/file_storage/test_generate_filename.py | 86 | ||||
| -rw-r--r-- | tests/model_fields/test_filefield.py | 10 |
7 files changed, 134 insertions, 16 deletions
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 b16fd05f5b..28d476224c 100644 --- a/django/db/models/fields/files.py +++ b/django/db/models/fields/files.py @@ -319,12 +319,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/index.txt b/docs/releases/index.txt index 93bc8248b7..7d75e28b5e 100644 --- a/docs/releases/index.txt +++ b/docs/releases/index.txt @@ -25,6 +25,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 @@ -63,6 +64,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 d4e70d6041..c269b7c035 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 @@ -62,6 +63,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') |