티스토리 수익 글 보기

티스토리 수익 글 보기

[3.1.x] Fixed CVE-2021-31542 — Tightened path & file name sanitation… · django/django@25d84d6 · GitHub
Skip to content
/ django Public

Commit 25d84d6

Browse files
apollo13carltongibson
apollo13
authored and
carltongibson
committed
[3.1.x] Fixed CVE-2021-31542 — Tightened path & file name sanitation in file uploads.
1 parent 6b0c7e6 commit 25d84d6

File tree

13 files changed

+178
–11
lines changed

13 files changed

+178
–11
lines changed

‎django/core/files/storage.py‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
import os
2+
import pathlib
23
from datetime import datetime
34
from urllib.parse import urljoin
45

56
from django.conf import settings
67
from django.core.exceptions import SuspiciousFileOperation
78
from django.core.files import File, locks
89
from django.core.files.move import file_move_safe
10+
from django.core.files.utils import validate_file_name
911
from django.core.signals import setting_changed
1012
from django.utils import timezone
1113
from django.utils._os import safe_join
@@ -74,6 +76,9 @@ def get_available_name(self, name, max_length=None):
7476
available for new content to be written to.
7577
"""
7678
dir_name, file_name = os.path.split(name)
79+
if '..' in pathlib.PurePath(dir_name).parts:
80+
raise SuspiciousFileOperation("Detected path traversal attempt in '%s'" % dir_name)
81+
validate_file_name(file_name)
7782
file_root, file_ext = os.path.splitext(file_name)
7883
# If the filename already exists, generate an alternative filename
7984
# until it doesn't exist.
@@ -105,6 +110,8 @@ def generate_filename(self, filename):
105110
"""
106111
# `filename` may include a path as returned by FileField.upload_to.
107112
dirname, filename = os.path.split(filename)
113+
if '..' in pathlib.PurePath(dirname).parts:
114+
raise SuspiciousFileOperation("Detected path traversal attempt in '%s'" % dirname)
108115
return os.path.normpath(os.path.join(dirname, self.get_valid_name(filename)))
109116

110117
def path(self, name):

‎django/core/files/uploadedfile.py‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
from django.conf import settings
99
from django.core.files import temp as tempfile
1010
from django.core.files.base import File
11+
from django.core.files.utils import validate_file_name
1112

1213
__all__ = ('UploadedFile', 'TemporaryUploadedFile', 'InMemoryUploadedFile',
1314
'SimpleUploadedFile')
@@ -47,6 +48,8 @@ def _set_name(self, name):
4748
ext = ext[:255]
4849
name = name[:255 - len(ext)] + ext
4950

51+
name = validate_file_name(name)
52+
5053
self._name = name
5154

5255
name = property(_get_name, _set_name)

‎django/core/files/utils.py‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,19 @@
1+
import os
2+
3+
from django.core.exceptions import SuspiciousFileOperation
4+
5+
6+
def validate_file_name(name):
7+
if name != os.path.basename(name):
8+
raise SuspiciousFileOperation("File name '%s' includes path elements" % name)
9+
10+
# Remove potentially dangerous names
11+
if name in {'', '.', '..'}:
12+
raise SuspiciousFileOperation("Could not derive file name from '%s'" % name)
13+
14+
return name
15+
16+
117
class FileProxyMixin:
218
"""
319
A mixin class used to forward file methods to an underlaying file

‎django/db/models/fields/files.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
from django.core.files.base import File
77
from django.core.files.images import ImageFile
88
from django.core.files.storage import Storage, default_storage
9+
from django.core.files.utils import validate_file_name
910
from django.db.models import signals
1011
from django.db.models.fields import Field
1112
from django.utils.translation import gettext_lazy as _
@@ -318,6 +319,7 @@ def generate_filename(self, instance, filename):
318319
Until the storage layer, all file paths are expected to be Unix style
319320
(with forward slashes).
320321
"""
322+
filename = validate_file_name(filename)
321323
if callable(self.upload_to):
322324
filename = self.upload_to(instance, filename)
323325
else:

‎django/http/multipartparser.py‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
import cgi
1010
import collections
1111
import html
12-
import os
1312
from urllib.parse import unquote
1413

1514
from django.conf import settings
@@ -299,10 +298,25 @@ def handle_file_complete(self, old_field_name, counters):
299298
break
300299

301300
def sanitize_file_name(self, file_name):
301+
"""
302+
Sanitize the filename of an upload.
303+
304+
Remove all possible path separators, even though that might remove more
305+
than actually required by the target system. Filenames that could
306+
potentially cause problems (current/parent dir) are also discarded.
307+
308+
It should be noted that this function could still return a "filepath"
309+
like "C:some_file.txt" which is handled later on by the storage layer.
310+
So while this function does sanitize filenames to some extent, the
311+
resulting filename should still be considered as untrusted user input.
312+
"""
302313
file_name = html.unescape(file_name)
303-
# Cleanup Windows-style path separators.
304-
file_name = file_name[file_name.rfind('\\') + 1:].strip()
305-
return os.path.basename(file_name)
314+
file_name = file_name.rsplit('/')[-1]
315+
file_name = file_name.rsplit('\\')[-1]
316+
317+
if file_name in {'', '.', '..'}:
318+
return None
319+
return file_name
306320

307321
IE_sanitize = sanitize_file_name
308322

‎django/utils/text.py‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
from gzip import GzipFile
66
from io import BytesIO
77

8+
from django.core.exceptions import SuspiciousFileOperation
89
from django.utils.deprecation import RemovedInDjango40Warning
910
from django.utils.functional import SimpleLazyObject, keep_lazy_text, lazy
1011
from django.utils.regex_helper import _lazy_re_compile
@@ -219,7 +220,7 @@ def _truncate_html(self, length, truncate, text, truncate_len, words):
219220

220221

221222
@keep_lazy_text
222-
def get_valid_filename(s):
223+
def get_valid_filename(name):
223224
"""
224225
Return the given string converted to a string that can be used for a clean
225226
filename. Remove leading and trailing spaces; convert other spaces to
@@ -228,8 +229,11 @@ def get_valid_filename(s):
228229
>>> get_valid_filename("john's portrait in 2004.jpg")
229230
'johns_portrait_in_2004.jpg'
230231
"""
231-
s = str(s).strip().replace(' ', '_')
232-
return re.sub(r'(?u)[^-\w.]', '', s)
232+
s = str(name).strip().replace(' ', '_')
233+
s = re.sub(r'(?u)[^-\w.]', '', s)
234+
if s in {'', '.', '..'}:
235+
raise SuspiciousFileOperation("Could not derive file name from '%s'" % name)
236+
return s
233237

234238

235239
@keep_lazy_text

‎docs/releases/2.2.21.txt‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
===========================
2+
Django 2.2.21 release notes
3+
===========================
4+
5+
*May 4, 2021*
6+
7+
Django 2.2.21 fixes a security issue in 2.2.20.
8+
9+
CVE-2021-31542: Potential directory-traversal via uploaded files
10+
================================================================
11+
12+
``MultiPartParser``, ``UploadedFile``, and ``FieldFile`` allowed
13+
directory-traversal via uploaded files with suitably crafted file names.
14+
15+
In order to mitigate this risk, stricter basename and path sanitation is now
16+
applied. Specifically, empty file names and paths with dot segments will be
17+
rejected.

‎docs/releases/3.1.9.txt‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
==========================
2+
Django 3.1.9 release notes
3+
==========================
4+
5+
*May 4, 2021*
6+
7+
Django 3.1.9 fixes a security issue in 3.1.8.
8+
9+
CVE-2021-31542: Potential directory-traversal via uploaded files
10+
================================================================
11+
12+
``MultiPartParser``, ``UploadedFile``, and ``FieldFile`` allowed
13+
directory-traversal via uploaded files with suitably crafted file names.
14+
15+
In order to mitigate this risk, stricter basename and path sanitation is now
16+
applied. Specifically, empty file names and paths with dot segments will be
17+
rejected.

‎docs/releases/index.txt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ versions of the documentation contain the release notes for any later releases.
2525
.. toctree::
2626
:maxdepth: 1
2727

28+
3.1.9
2829
3.1.8
2930
3.1.7
3031
3.1.6
@@ -61,6 +62,7 @@ versions of the documentation contain the release notes for any later releases.
6162
.. toctree::
6263
:maxdepth: 1
6364

65+
2.2.21
6466
2.2.20
6567
2.2.19
6668
2.2.18

‎tests/file_storage/test_generate_filename.py‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
import os
22

3+
from django.core.exceptions import SuspiciousFileOperation
34
from django.core.files.base import ContentFile
4-
from django.core.files.storage import Storage
5+
from django.core.files.storage import FileSystemStorage, Storage
56
from django.db.models import FileField
67
from django.test import SimpleTestCase
78

@@ -36,6 +37,44 @@ def generate_filename(self, filename):
3637

3738

3839
class GenerateFilenameStorageTests(SimpleTestCase):
40+
def test_storage_dangerous_paths(self):
41+
candidates = [
42+
('/tmp/..', '..'),
43+
('/tmp/.', '.'),
44+
('', ''),
45+
]
46+
s = FileSystemStorage()
47+
msg = "Could not derive file name from '%s'"
48+
for file_name, base_name in candidates:
49+
with self.subTest(file_name=file_name):
50+
with self.assertRaisesMessage(SuspiciousFileOperation, msg % base_name):
51+
s.get_available_name(file_name)
52+
with self.assertRaisesMessage(SuspiciousFileOperation, msg % base_name):
53+
s.generate_filename(file_name)
54+
55+
def test_storage_dangerous_paths_dir_name(self):
56+
file_name = '/tmp/../path'
57+
s = FileSystemStorage()
58+
msg = "Detected path traversal attempt in '/tmp/..'"
59+
with self.assertRaisesMessage(SuspiciousFileOperation, msg):
60+
s.get_available_name(file_name)
61+
with self.assertRaisesMessage(SuspiciousFileOperation, msg):
62+
s.generate_filename(file_name)
63+
64+
def test_filefield_dangerous_filename(self):
65+
candidates = ['..', '.', '', '???', '$.$.$']
66+
f = FileField(upload_to='some/folder/')
67+
msg = "Could not derive file name from '%s'"
68+
for file_name in candidates:
69+
with self.subTest(file_name=file_name):
70+
with self.assertRaisesMessage(SuspiciousFileOperation, msg % file_name):
71+
f.generate_filename(None, file_name)
72+
73+
def test_filefield_dangerous_filename_dir(self):
74+
f = FileField(upload_to='some/folder/')
75+
msg = "File name '/tmp/path' includes path elements"
76+
with self.assertRaisesMessage(SuspiciousFileOperation, msg):
77+
f.generate_filename(None, '/tmp/path')
3978

4079
def test_filefield_generate_filename(self):
4180
f = FileField(upload_to='some/folder/')

0 commit comments

Comments
 (0)