[3.1.x] Fixed CVE-2021-31542 -- Tightened path & file name sanitation in file uploads.
This commit is contained in:
parent
6b0c7e6f50
commit
25d84d6412
|
@ -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):
|
||||
|
|
|
@ -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)
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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.utils.translation import gettext_lazy as _
|
||||
|
@ -318,6 +319,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:
|
||||
|
|
|
@ -9,7 +9,6 @@ import binascii
|
|||
import cgi
|
||||
import collections
|
||||
import html
|
||||
import os
|
||||
from urllib.parse import unquote
|
||||
|
||||
from django.conf import settings
|
||||
|
@ -299,10 +298,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
|
||||
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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.
|
|
@ -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.
|
|
@ -25,6 +25,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
|
||||
|
@ -61,6 +62,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
|
||||
|
|
|
@ -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/')
|
||||
|
|
|
@ -8,8 +8,9 @@ import unittest
|
|||
from io import BytesIO, StringIO
|
||||
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 (
|
||||
MultiPartParser, MultiPartParserError, parse_header,
|
||||
)
|
||||
|
@ -38,6 +39,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):
|
||||
|
@ -52,6 +63,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 = {
|
||||
|
@ -685,6 +712,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",
|
||||
|
|
|
@ -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')
|
||||
|
|
|
@ -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
|
||||
|
@ -243,6 +244,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)]
|
||||
|
|
Loading…
Reference in New Issue