From c98f446c188596d4ba6de71d1b77b4a6c5c2a007 Mon Sep 17 00:00:00 2001 From: Florian Apolloner Date: Wed, 14 Apr 2021 18:23:44 +0200 Subject: [PATCH] [3.2.x] Fixed CVE-2021-31542 -- Tightened path & file name sanitation in file uploads. --- django/core/files/storage.py | 7 ++++ django/core/files/uploadedfile.py | 3 ++ django/core/files/utils.py | 16 ++++++++ django/db/models/fields/files.py | 2 + django/http/multipartparser.py | 22 ++++++++-- django/utils/text.py | 10 +++-- docs/releases/2.2.21.txt | 17 ++++++++ docs/releases/3.1.9.txt | 17 ++++++++ docs/releases/3.2.1.txt | 14 ++++++- docs/releases/index.txt | 2 + tests/file_storage/test_generate_filename.py | 41 ++++++++++++++++++- tests/file_uploads/tests.py | 38 ++++++++++++++++- .../forms_tests/field_tests/test_filefield.py | 6 ++- tests/utils_tests/test_text.py | 8 ++++ 14 files changed, 190 insertions(+), 13 deletions(-) create mode 100644 docs/releases/2.2.21.txt create mode 100644 docs/releases/3.1.9.txt diff --git a/django/core/files/storage.py b/django/core/files/storage.py index 16f9d4e27b1..3e68853b59f 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 48007b86823..f452bcd9a4a 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 de896071759..f83cb1a3cfe 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 d410771cf3e..a2f972489f8 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 180a533bb6a..f464caa1b4c 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 4d77ce7f41c..baa44f279e8 100644 --- a/django/utils/text.py +++ b/django/utils/text.py @@ -5,6 +5,7 @@ import warnings from gzip import GzipFile from io import BytesIO +from django.core.exceptions import SuspiciousFileOperation from django.utils.deprecation import RemovedInDjango40Warning from django.utils.functional import SimpleLazyObject, keep_lazy_text, lazy from django.utils.regex_helper import _lazy_re_compile @@ -219,7 +220,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 @@ -228,8 +229,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 00000000000..f32aeadff76 --- /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 00000000000..682270b9016 --- /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 545c9adce3a..97ac4ebc94b 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 ca1d1ed8eaa..ea53ca00634 100644 --- a/docs/releases/index.txt +++ b/docs/releases/index.txt @@ -33,6 +33,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 @@ -69,6 +70,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 b4222f41216..9f54f6921e2 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 e8f91e2fa04..7bc8d41dac8 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 261d9f4ca96..2db106e4a0d 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 1b6bfc0b8e4..d2a94fcdabe 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, ignore_warnings from django.utils import text from django.utils.deprecation import RemovedInDjango40Warning @@ -255,6 +256,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)]