Skip to content

Commit

Permalink
[3.1.x] Fixed #32718 -- Relaxed file name validation in FileField.
Browse files Browse the repository at this point in the history
- 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 0b79eb3.

Backport of b556999 from main.
  • Loading branch information
felixxm committed May 13, 2021
1 parent 9fb9944 commit b7d4a6f
Show file tree
Hide file tree
Showing 7 changed files with 134 additions and 16 deletions.
20 changes: 15 additions & 5 deletions django/core/files/utils.py
Original file line number Diff line number Diff line change
@@ -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


Expand Down
2 changes: 1 addition & 1 deletion django/db/models/fields/files.py
Original file line number Diff line number Diff line change
Expand Up @@ -319,12 +319,12 @@ def generate_filename(self, instance, filename):
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):
Expand Down
15 changes: 15 additions & 0 deletions docs/releases/2.2.23.txt
Original file line number Diff line number Diff line change
@@ -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`).
15 changes: 15 additions & 0 deletions docs/releases/3.1.11.txt
Original file line number Diff line number Diff line change
@@ -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`).
2 changes: 2 additions & 0 deletions docs/releases/index.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
86 changes: 76 additions & 10 deletions tests/file_storage/test_generate_filename.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -64,19 +62,37 @@ def test_storage_dangerous_paths_dir_name(self):
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/')
Expand All @@ -95,7 +111,57 @@ def upload_to(instance, filename):
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
Expand Down
10 changes: 10 additions & 0 deletions tests/model_fields/test_filefield.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
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
Expand Down Expand Up @@ -62,6 +63,15 @@ def test_refresh_from_db(self):
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')
Expand Down

0 comments on commit b7d4a6f

Please sign in to comment.