summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--django/core/files/storage.py7
-rw-r--r--django/core/files/uploadedfile.py3
-rw-r--r--django/core/files/utils.py16
-rw-r--r--django/db/models/fields/files.py2
-rw-r--r--django/http/multipartparser.py22
-rw-r--r--django/utils/text.py10
-rw-r--r--docs/releases/2.2.21.txt17
-rw-r--r--docs/releases/3.1.9.txt17
-rw-r--r--docs/releases/3.2.1.txt14
-rw-r--r--docs/releases/index.txt2
-rw-r--r--tests/file_storage/test_generate_filename.py41
-rw-r--r--tests/file_uploads/tests.py38
-rw-r--r--tests/forms_tests/field_tests/test_filefield.py6
-rw-r--r--tests/utils_tests/test_text.py8
14 files changed, 190 insertions, 13 deletions
diff --git a/django/core/files/storage.py b/django/core/files/storage.py
index 29b0e4c9ed..8190791f9a 100644
--- a/django/core/files/storage.py
+++ b/django/core/files/storage.py
@@ -1,4 +1,5 @@
import os
+import pathlib
from datetime import datetime
from urllib.parse import urljoin
@@ -6,6 +7,7 @@ from django.conf import settings
from django.core.exceptions import SuspiciousFileOperation
from django.core.files import File, locks
from django.core.files.move import file_move_safe
+from django.core.files.utils import validate_file_name
from django.core.signals import setting_changed
from django.utils import timezone
from django.utils._os import safe_join
@@ -74,6 +76,9 @@ class Storage:
available for new content to be written to.
"""
dir_name, file_name = os.path.split(name)
+ if '..' in pathlib.PurePath(dir_name).parts:
+ raise SuspiciousFileOperation("Detected path traversal attempt in '%s'" % dir_name)
+ validate_file_name(file_name)
file_root, file_ext = os.path.splitext(file_name)
# If the filename already exists, generate an alternative filename
# until it doesn't exist.
@@ -105,6 +110,8 @@ class Storage:
"""
# `filename` may include a path as returned by FileField.upload_to.
dirname, filename = os.path.split(filename)
+ if '..' in pathlib.PurePath(dirname).parts:
+ raise SuspiciousFileOperation("Detected path traversal attempt in '%s'" % dirname)
return os.path.normpath(os.path.join(dirname, self.get_valid_name(filename)))
def path(self, name):
diff --git a/django/core/files/uploadedfile.py b/django/core/files/uploadedfile.py
index 48007b8682..f452bcd9a4 100644
--- a/django/core/files/uploadedfile.py
+++ b/django/core/files/uploadedfile.py
@@ -8,6 +8,7 @@ from io import BytesIO
from django.conf import settings
from django.core.files import temp as tempfile
from django.core.files.base import File
+from django.core.files.utils import validate_file_name
__all__ = ('UploadedFile', 'TemporaryUploadedFile', 'InMemoryUploadedFile',
'SimpleUploadedFile')
@@ -47,6 +48,8 @@ class UploadedFile(File):
ext = ext[:255]
name = name[:255 - len(ext)] + ext
+ name = validate_file_name(name)
+
self._name = name
name = property(_get_name, _set_name)
diff --git a/django/core/files/utils.py b/django/core/files/utils.py
index de89607175..f83cb1a3cf 100644
--- a/django/core/files/utils.py
+++ b/django/core/files/utils.py
@@ -1,3 +1,19 @@
+import os
+
+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)
+
+ # Remove potentially dangerous names
+ if name in {'', '.', '..'}:
+ raise SuspiciousFileOperation("Could not derive file name from '%s'" % name)
+
+ return name
+
+
class FileProxyMixin:
"""
A mixin class used to forward file methods to an underlaying file
diff --git a/django/db/models/fields/files.py b/django/db/models/fields/files.py
index d410771cf3..a2f972489f 100644
--- a/django/db/models/fields/files.py
+++ b/django/db/models/fields/files.py
@@ -6,6 +6,7 @@ from django.core import checks
from django.core.files.base import File
from django.core.files.images import ImageFile
from django.core.files.storage import Storage, default_storage
+from django.core.files.utils import validate_file_name
from django.db.models import signals
from django.db.models.fields import Field
from django.db.models.query_utils import DeferredAttribute
@@ -312,6 +313,7 @@ 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:
diff --git a/django/http/multipartparser.py b/django/http/multipartparser.py
index 180a533bb6..f464caa1b4 100644
--- a/django/http/multipartparser.py
+++ b/django/http/multipartparser.py
@@ -9,7 +9,6 @@ import binascii
import cgi
import collections
import html
-import os
from urllib.parse import unquote
from django.conf import settings
@@ -306,10 +305,25 @@ class MultiPartParser:
break
def sanitize_file_name(self, file_name):
+ """
+ Sanitize the filename of an upload.
+
+ Remove all possible path separators, even though that might remove more
+ than actually required by the target system. Filenames that could
+ potentially cause problems (current/parent dir) are also discarded.
+
+ It should be noted that this function could still return a "filepath"
+ like "C:some_file.txt" which is handled later on by the storage layer.
+ So while this function does sanitize filenames to some extent, the
+ resulting filename should still be considered as untrusted user input.
+ """
file_name = html.unescape(file_name)
- # Cleanup Windows-style path separators.
- file_name = file_name[file_name.rfind('\\') + 1:].strip()
- return os.path.basename(file_name)
+ file_name = file_name.rsplit('/')[-1]
+ file_name = file_name.rsplit('\\')[-1]
+
+ if file_name in {'', '.', '..'}:
+ return None
+ return file_name
IE_sanitize = sanitize_file_name
diff --git a/django/utils/text.py b/django/utils/text.py
index 7cc388f7fe..7f3368b16a 100644
--- a/django/utils/text.py
+++ b/django/utils/text.py
@@ -4,6 +4,7 @@ import unicodedata
from gzip import GzipFile
from io import BytesIO
+from django.core.exceptions import SuspiciousFileOperation
from django.utils.functional import SimpleLazyObject, keep_lazy_text, lazy
from django.utils.regex_helper import _lazy_re_compile
from django.utils.translation import gettext as _, gettext_lazy, pgettext
@@ -221,7 +222,7 @@ class Truncator(SimpleLazyObject):
@keep_lazy_text
-def get_valid_filename(s):
+def get_valid_filename(name):
"""
Return the given string converted to a string that can be used for a clean
filename. Remove leading and trailing spaces; convert other spaces to
@@ -230,8 +231,11 @@ def get_valid_filename(s):
>>> get_valid_filename("john's portrait in 2004.jpg")
'johns_portrait_in_2004.jpg'
"""
- s = str(s).strip().replace(' ', '_')
- return re.sub(r'(?u)[^-\w.]', '', s)
+ s = str(name).strip().replace(' ', '_')
+ s = re.sub(r'(?u)[^-\w.]', '', s)
+ if s in {'', '.', '..'}:
+ raise SuspiciousFileOperation("Could not derive file name from '%s'" % name)
+ return s
@keep_lazy_text
diff --git a/docs/releases/2.2.21.txt b/docs/releases/2.2.21.txt
new file mode 100644
index 0000000000..f32aeadff7
--- /dev/null
+++ b/docs/releases/2.2.21.txt
@@ -0,0 +1,17 @@
+===========================
+Django 2.2.21 release notes
+===========================
+
+*May 4, 2021*
+
+Django 2.2.21 fixes a security issue in 2.2.20.
+
+CVE-2021-31542: Potential directory-traversal via uploaded files
+================================================================
+
+``MultiPartParser``, ``UploadedFile``, and ``FieldFile`` allowed
+directory-traversal via uploaded files with suitably crafted file names.
+
+In order to mitigate this risk, stricter basename and path sanitation is now
+applied. Specifically, empty file names and paths with dot segments will be
+rejected.
diff --git a/docs/releases/3.1.9.txt b/docs/releases/3.1.9.txt
new file mode 100644
index 0000000000..682270b901
--- /dev/null
+++ b/docs/releases/3.1.9.txt
@@ -0,0 +1,17 @@
+==========================
+Django 3.1.9 release notes
+==========================
+
+*May 4, 2021*
+
+Django 3.1.9 fixes a security issue in 3.1.8.
+
+CVE-2021-31542: Potential directory-traversal via uploaded files
+================================================================
+
+``MultiPartParser``, ``UploadedFile``, and ``FieldFile`` allowed
+directory-traversal via uploaded files with suitably crafted file names.
+
+In order to mitigate this risk, stricter basename and path sanitation is now
+applied. Specifically, empty file names and paths with dot segments will be
+rejected.
diff --git a/docs/releases/3.2.1.txt b/docs/releases/3.2.1.txt
index 545c9adce3..97ac4ebc94 100644
--- a/docs/releases/3.2.1.txt
+++ b/docs/releases/3.2.1.txt
@@ -2,9 +2,19 @@
Django 3.2.1 release notes
==========================
-*Expected May 4, 2021*
+*May 4, 2021*
-Django 3.2.1 fixes several bugs in 3.2.
+Django 3.2.1 fixes a security issue and several bugs in 3.2.
+
+CVE-2021-31542: Potential directory-traversal via uploaded files
+================================================================
+
+``MultiPartParser``, ``UploadedFile``, and ``FieldFile`` allowed
+directory-traversal via uploaded files with suitably crafted file names.
+
+In order to mitigate this risk, stricter basename and path sanitation is now
+applied. Specifically, empty file names and paths with dot segments will be
+rejected.
Bugfixes
========
diff --git a/docs/releases/index.txt b/docs/releases/index.txt
index 0249642d87..6421478c9c 100644
--- a/docs/releases/index.txt
+++ b/docs/releases/index.txt
@@ -40,6 +40,7 @@ versions of the documentation contain the release notes for any later releases.
.. toctree::
:maxdepth: 1
+ 3.1.9
3.1.8
3.1.7
3.1.6
@@ -76,6 +77,7 @@ versions of the documentation contain the release notes for any later releases.
.. toctree::
:maxdepth: 1
+ 2.2.21
2.2.20
2.2.19
2.2.18
diff --git a/tests/file_storage/test_generate_filename.py b/tests/file_storage/test_generate_filename.py
index b4222f4121..9f54f6921e 100644
--- a/tests/file_storage/test_generate_filename.py
+++ b/tests/file_storage/test_generate_filename.py
@@ -1,7 +1,8 @@
import os
+from django.core.exceptions import SuspiciousFileOperation
from django.core.files.base import ContentFile
-from django.core.files.storage import Storage
+from django.core.files.storage import FileSystemStorage, Storage
from django.db.models import FileField
from django.test import SimpleTestCase
@@ -36,6 +37,44 @@ class AWSS3Storage(Storage):
class GenerateFilenameStorageTests(SimpleTestCase):
+ def test_storage_dangerous_paths(self):
+ candidates = [
+ ('/tmp/..', '..'),
+ ('/tmp/.', '.'),
+ ('', ''),
+ ]
+ s = FileSystemStorage()
+ msg = "Could not derive file name from '%s'"
+ for file_name, base_name in candidates:
+ with self.subTest(file_name=file_name):
+ with self.assertRaisesMessage(SuspiciousFileOperation, msg % base_name):
+ s.get_available_name(file_name)
+ with self.assertRaisesMessage(SuspiciousFileOperation, msg % base_name):
+ s.generate_filename(file_name)
+
+ def test_storage_dangerous_paths_dir_name(self):
+ file_name = '/tmp/../path'
+ s = FileSystemStorage()
+ msg = "Detected path traversal attempt in '/tmp/..'"
+ with self.assertRaisesMessage(SuspiciousFileOperation, msg):
+ s.get_available_name(file_name)
+ with self.assertRaisesMessage(SuspiciousFileOperation, msg):
+ s.generate_filename(file_name)
+
+ def test_filefield_dangerous_filename(self):
+ candidates = ['..', '.', '', '???', '$.$.$']
+ f = FileField(upload_to='some/folder/')
+ msg = "Could not derive file name from '%s'"
+ for file_name in candidates:
+ with self.subTest(file_name=file_name):
+ with self.assertRaisesMessage(SuspiciousFileOperation, msg % file_name):
+ f.generate_filename(None, file_name)
+
+ def test_filefield_dangerous_filename_dir(self):
+ f = FileField(upload_to='some/folder/')
+ msg = "File name '/tmp/path' includes path elements"
+ with self.assertRaisesMessage(SuspiciousFileOperation, msg):
+ f.generate_filename(None, '/tmp/path')
def test_filefield_generate_filename(self):
f = FileField(upload_to='some/folder/')
diff --git a/tests/file_uploads/tests.py b/tests/file_uploads/tests.py
index e8f91e2fa0..7bc8d41dac 100644
--- a/tests/file_uploads/tests.py
+++ b/tests/file_uploads/tests.py
@@ -9,8 +9,9 @@ from io import BytesIO, StringIO
from unittest import mock
from urllib.parse import quote
+from django.core.exceptions import SuspiciousFileOperation
from django.core.files import temp as tempfile
-from django.core.files.uploadedfile import SimpleUploadedFile
+from django.core.files.uploadedfile import SimpleUploadedFile, UploadedFile
from django.http.multipartparser import (
FILE, MultiPartParser, MultiPartParserError, Parser, parse_header,
)
@@ -39,6 +40,16 @@ CANDIDATE_TRAVERSAL_FILE_NAMES = [
'../hax0rd.txt', # HTML entities.
]
+CANDIDATE_INVALID_FILE_NAMES = [
+ '/tmp/', # Directory, *nix-style.
+ 'c:\\tmp\\', # Directory, win-style.
+ '/tmp/.', # Directory dot, *nix-style.
+ 'c:\\tmp\\.', # Directory dot, *nix-style.
+ '/tmp/..', # Parent directory, *nix-style.
+ 'c:\\tmp\\..', # Parent directory, win-style.
+ '', # Empty filename.
+]
+
@override_settings(MEDIA_ROOT=MEDIA_ROOT, ROOT_URLCONF='file_uploads.urls', MIDDLEWARE=[])
class FileUploadTests(TestCase):
@@ -53,6 +64,22 @@ class FileUploadTests(TestCase):
shutil.rmtree(MEDIA_ROOT)
super().tearDownClass()
+ def test_upload_name_is_validated(self):
+ candidates = [
+ '/tmp/',
+ '/tmp/..',
+ '/tmp/.',
+ ]
+ if sys.platform == 'win32':
+ candidates.extend([
+ 'c:\\tmp\\',
+ 'c:\\tmp\\..',
+ 'c:\\tmp\\.',
+ ])
+ for file_name in candidates:
+ with self.subTest(file_name=file_name):
+ self.assertRaises(SuspiciousFileOperation, UploadedFile, name=file_name)
+
def test_simple_upload(self):
with open(__file__, 'rb') as fp:
post_data = {
@@ -718,6 +745,15 @@ class MultiParserTests(SimpleTestCase):
with self.subTest(file_name=file_name):
self.assertEqual(parser.sanitize_file_name(file_name), 'hax0rd.txt')
+ def test_sanitize_invalid_file_name(self):
+ parser = MultiPartParser({
+ 'CONTENT_TYPE': 'multipart/form-data; boundary=_foo',
+ 'CONTENT_LENGTH': '1',
+ }, StringIO('x'), [], 'utf-8')
+ for file_name in CANDIDATE_INVALID_FILE_NAMES:
+ with self.subTest(file_name=file_name):
+ self.assertIsNone(parser.sanitize_file_name(file_name))
+
def test_rfc2231_parsing(self):
test_data = (
(b"Content-Type: application/x-stuff; title*=us-ascii'en-us'This%20is%20%2A%2A%2Afun%2A%2A%2A",
diff --git a/tests/forms_tests/field_tests/test_filefield.py b/tests/forms_tests/field_tests/test_filefield.py
index 261d9f4ca9..2db106e4a0 100644
--- a/tests/forms_tests/field_tests/test_filefield.py
+++ b/tests/forms_tests/field_tests/test_filefield.py
@@ -21,10 +21,12 @@ class FileFieldTest(SimpleTestCase):
f.clean(None, '')
self.assertEqual('files/test2.pdf', f.clean(None, 'files/test2.pdf'))
no_file_msg = "'No file was submitted. Check the encoding type on the form.'"
+ file = SimpleUploadedFile(None, b'')
+ file._name = ''
with self.assertRaisesMessage(ValidationError, no_file_msg):
- f.clean(SimpleUploadedFile('', b''))
+ f.clean(file)
with self.assertRaisesMessage(ValidationError, no_file_msg):
- f.clean(SimpleUploadedFile('', b''), '')
+ f.clean(file, '')
self.assertEqual('files/test3.pdf', f.clean(None, 'files/test3.pdf'))
with self.assertRaisesMessage(ValidationError, no_file_msg):
f.clean('some content that is not a file')
diff --git a/tests/utils_tests/test_text.py b/tests/utils_tests/test_text.py
index c9c74521e3..852a7970ee 100644
--- a/tests/utils_tests/test_text.py
+++ b/tests/utils_tests/test_text.py
@@ -1,6 +1,7 @@
import json
import sys
+from django.core.exceptions import SuspiciousFileOperation
from django.test import SimpleTestCase
from django.utils import text
from django.utils.functional import lazystr
@@ -228,6 +229,13 @@ class TestUtilsText(SimpleTestCase):
filename = "^&'@{}[],$=!-#()%+~_123.txt"
self.assertEqual(text.get_valid_filename(filename), "-_123.txt")
self.assertEqual(text.get_valid_filename(lazystr(filename)), "-_123.txt")
+ msg = "Could not derive file name from '???'"
+ with self.assertRaisesMessage(SuspiciousFileOperation, msg):
+ text.get_valid_filename('???')
+ # After sanitizing this would yield '..'.
+ msg = "Could not derive file name from '$.$.$'"
+ with self.assertRaisesMessage(SuspiciousFileOperation, msg):
+ text.get_valid_filename('$.$.$')
def test_compress_sequence(self):
data = [{'key': i} for i in range(10)]
View Lorry Controller Status