8000 [1.5.x] Fixed #23157 -- Removed O(n) algorithm when uploading duplica… · alex-python/django@26cd48e · GitHub
[go: up one dir, main page]
Skip to content

Commit 26cd48e

Browse files
timgrahamtimgraham
committed
[1.5.x] Fixed #23157 -- Removed O(n) algorithm when uploading duplicate file names.
This is a security fix. Disclosure following shortly.
1 parent 45ac9d4 commit 26cd48e

File tree

7 files changed

+93
-25
lines changed

7 files changed

+93
-25
lines changed

‎django/core/files/storage.py

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,13 @@
44
from urllib.parse import urljoin
55
except ImportError: # Python 2
66
from urlparse import urljoin
7-
import itertools
87
from datetime import datetime
98

109
from django.conf import settings
1110
from django.core.exceptions import ImproperlyConfigured, SuspiciousOperation
1211
from django.core.files import locks, File
1312
from django.core.files.move import file_move_safe
13+
from django.utils.crypto import get_random_string
1414
from django.utils.encoding import force_text, filepath_to_uri
1515
from django.utils.functional import LazyObject
1616
from django.utils.importlib import import_module
@@ -66,13 +66,12 @@ def get_available_name(self, name):
6666
"""
6767
dir_name, file_name = os.path.split(name)
6868
file_root, file_ext = os.path.splitext(file_name)
69-
# If the filename already exists, add an underscore and a number (before
70-
# the file extension, if one exists) to the filename until the generated
71-
# filename doesn't exist.
72-
count = itertools.count(1)
69+
# If the filename already exists, add an underscore and a random 7
70+
# character alphanumeric string (before the file extension, if one
71+
# exists) to the filename until the generated filename doesn't exist.
7372
while self.exists(name):
7473
# file_ext includes the dot.
75-
name = os.path.join(dir_name, "%s_%s%s" % (file_root, next(count), file_ext))
74+
name = os.path.join(dir_name, "%s_%s%s" % (file_root, get_random_string(7), file_ext))
7675

7776
return name
7877

‎docs/howto/custom-file-storage.txt

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,5 +83,14 @@ the provided filename into account. The ``name`` argument passed to this method
8383
will have already cleaned to a filename valid for the storage system, according
8484
to the ``get_valid_name()`` method described above.
8585

86-
The code provided on ``Storage`` simply appends ``"_1"``, ``"_2"``, etc. to the
87-
filename until it finds one that's available in the destination directory.
86+
.. versionchanged:: 1.5.9
87+
88+
If a file with ``name`` already exists, an underscore plus a random 7
89+
character alphanumeric string is appended to the filename before the
90+
extension.
91+
92+
Previously, an underscore followed by a number (e.g. ``"_1"``, ``"_2"``,
93+
etc.) was appended to the filename until an avaible name in the destination
94+
directory was found. A malicious user could exploit this deterministic
95+
algorithm to create a denial-of-service attack. This change was also made
96+
in Django 1.4.14.

‎docs/ref/files/storage.txt

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,17 @@ The Storage Class
8181
available for new content to be written to on the target storage
8282
system.
8383

84+
.. versionchanged:: 1.5.9
85+
86+
If a file with ``name`` already exists, an underscore plus a random 7
87+
character alphanumeric string is appended to the filename before the
88+
extension.
89+
90+
Previously, an underscore followed by a number (e.g. ``"_1"``, ``"_2"``,
91+
etc.) was appended to the filename until an avaible name in the
92+
destination directory was found. A malicious user could exploit this
93+
deterministic algorithm to create a denial-of-service attack. This
94+
change was also made in Django 1.4.14.
8495

8596
.. method:: get_valid_name(name)
8697

‎docs/releases/1.4.14.txt

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,3 +18,23 @@ To remedy this, URL reversing now ensures that no URL starts with two slashes
1818
(//), replacing the second slash with its URL encoded counterpart (%2F). This
1919
approach ensures that semantics stay the same, while making the URL relative to
2020
the domain and not to the scheme.
21+
22+
File upload denial-of-service
23+
=============================
24+
25+
Before this release, Django's file upload handing in its default configuration
26+
may degrade to producing a huge number of ``os.stat()`` system calls when a
27+
duplicate filename is uploaded. Since ``stat()`` may invoke IO, this may produce
28+
a huge data-dependent slowdown that slowly worsens over time. The net result is
29+
that given enough time, a user with the ability to upload files can cause poor
30+
performance in the upload handler, eventually causing it to become very slow
31+
simply by uploading 0-byte files. At this point, even a slow network connection
32+
and few HTTP requests would be all that is necessary to make a site unavailable.
33+
34+
We've remedied the issue by changing the algorithm for generating file names
35+
if a file with the uploaded name already exists.
36+
:meth:`Storage.get_available_name()
37+
<django.core.files.storage.Storage.get_available_name>` now appends an
38+
underscore plus a random 7 character alphanumeric string (e.g. ``"_x3a1gho"``),
39+
rather than iterating through an underscore followed by a number (e.g. ``"_1"``,
40+
``"_2"``, etc.).

‎docs/releases/1.5.9.txt

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,3 +18,23 @@ To remedy this, URL reversing now ensures that no URL starts with two slashes
1818
(//), replacing the second slash with its URL encoded counterpart (%2F). This
1919
approach ensures that semantics stay the same, while making the URL relative to
2020
the domain and not to the scheme.
21+
22+
File upload denial-of-service
23+
=============================
24+
25+
Before this release, Django's file upload handing in its default configuration
26+
may degrade to producing a huge number of ``os.stat()`` system calls when a
27+
duplicate filename is uploaded. Since ``stat()`` may invoke IO, this may produce
28+
a huge data-dependent slowdown that slowly worsens over time. The net result is
29+
that given enough time, a user with the ability to upload files can cause poor
30+
performance in the upload handler, eventually causing it to become very slow
31+
simply by uploading 0-byte files. At this point, even a slow network connection
32+
and few HTTP requests would be all that is necessary to make a site unavailable.
33+
34+
We've remedied the issue by changing the algorithm for generating file names
35+
if a file with the uploaded name already exists.
36+
:meth:`Storage.get_available_name()
37+
<django.core.files.storage.Storage.get_available_name>` now appends an
38+
underscore plus a random 7 character alphanumeric string (e.g. ``"_x3a1gho"``),
39+
rather than iterating through an underscore followed by a number (e.g. ``"_1"``,
40+
``"_2"``, etc.).

‎tests/modeltests/files/tests.py

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,14 @@
99
from django.core.files.base import ContentFile
1010
from django.core.files.uploadedfile import SimpleUploadedFile
1111
from django.test import TestCase
12-
from django.utils import unittest
12+
from django.utils import six, unittest
1313

1414
from .models import Storage, temp_storage, temp_storage_location
1515

1616

17+
FILE_SUFFIX_REGEX = '[A-Za-z0-9]{7}'
18+
19+
1720
class FileStorageTests(TestCase):
1821
def tearDown(self):
1922
shutil.rmtree(temp_storage_location)
@@ -59,27 +62,28 @@ def test_files(self):
5962
# Save another file with the same name.
6063
obj2 = Storage()
6164
obj2.normal.save("django_test.txt", ContentFile("more content"))
62-
self.assertEqual(obj2.normal.name, "tests/django_test_1.txt")
65+
obj2_name = obj2.normal.name
66+
six.assertRegex(self, obj2_name, "tests/django_test_%s.txt" % FILE_SUFFIX_REGEX)
6367
self.assertEqual(obj2.normal.size, 12)
6468

6569
# Push the objects into the cache to make sure they pickle properly
6670
cache.set("obj1", obj1)
6771
cache.set("obj2", obj2)
68-
self.assertEqual(cache.get("obj2").normal.name, "tests/django_test_1.txt")
72+
six.assertRegex(self, cache.get("obj2").normal.name, "tests/django_test_%s.txt" % FILE_SUFFIX_REGEX)
6973

7074
# Deleting an object does not delete the file it uses.
7175
obj2.delete()
7276
obj2.normal.save("django_test.txt", ContentFile("more content"))
73-
self.assertEqual(obj2.normal.name, "tests/django_test_2.txt")
77+
self.assertNotEqual(obj2_name, obj2.normal.name)
78+
six.assertRegex(self, obj2.normal.name, "tests/django_test_%s.txt" % FILE_SUFFIX_REGEX)
7479

7580
# Multiple files with the same name get _N appended to them.
76-
objs = [Storage() for i in range(3)]
81+
objs = [Storage() for i in range(2)]
7782
for o in objs:
7883
o.normal.save("multiple_files.txt", ContentFile("Same Content"))
79-
self.assertEqual(
80-
[o.normal.name for o in objs],
81-
["tests/multiple_files.txt", "tests/multiple_files_1.txt", "tests/multiple_files_2.txt"]
82-
)
84+
names = [o.normal.name for o in objs]
85+
self.assertEqual(names[0], "tests/multiple_files.txt")
86+
six.assertRegex(self, names[1], "tests/multiple_files_%s.txt" % FILE_SUFFIX_REGEX)
8387
for o in objs:
8488
o.delete()
8589

‎tests/regressiontests/file_storage/tests.py

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@
4040
Image = None
4141

4242

43+
FILE_SUFFIX_REGEX = '[A-Za-z0-9]{7}'
44+
45+
4346
class GetStorageClassTests(SimpleTestCase):
4447

4548
def test_get_filesystem_storage(self):
@@ -431,10 +434,9 @@ def test_race_condition(self):
431434
self.thread.start()
432435
name = self.save_file('conflict')
433436
self.thread.join()
434-
self.assertTrue(self.storage.exists('conflict'))
435-
self.assertTrue(self.storage.exists('conflict_1'))
436-
self.storage.delete('conflict')
437-
self.storage.delete('conflict_1')
437+
files = sorted(os.listdir(self.storage_dir))
438+
self.assertEqual(files[0], 'conflict')
439+
six.assertRegex(self, files[1], 'conflict_%s' % FILE_SUFFIX_REGEX)
438440

439441
@unittest.skipIf(sys.platform.startswith('win'), "Windows only partially supports umasks and chmod.")
440442
class FileStoragePermissions(unittest.TestCase):
@@ -478,9 +480,10 @@ def test_directory_with_dot(self):
478480
self.storage.save('dotted.path/test', ContentFile("1"))
479481
self.storage.save('dotted.path/test', ContentFile("2"))
480482

483+
files = sorted(os.listdir(os.path.join(self.storage_dir, 'dotted.path')))
481484
self.assertFalse(os.path.exists(os.path.join(self.storage_dir, 'dotted_.path')))
482-
self.assertTrue(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/test')))
483-
self.assertTrue(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/test_1')))
485+
self.assertEqual(files[0], 'test')
486+
six.assertRegex(self, files[1], 'test_%s' % FILE_SUFFIX_REGEX)
484487

485488
def test_first_character_dot(self):
486489
"""
@@ -490,8 +493,10 @@ def test_first_character_dot(self):
490493
self.storage.save('dotted.path/.test', ContentFile("1"))
491494
self.storage.save('dotted.path/.test', ContentFile("2"))
492495

493-
self.assertTrue(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/.test')))
494-
self.assertTrue(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/.test_1')))
496+
files = sorted(os.listdir(os.path.join(self.storage_dir, 'dotted.path')))
497+
self.assertFalse(os.path.exists(os.path.join(self.storage_dir, 'dotted_.path')))
498+
self.assertEqual(files[0], '.test')
499+
six.assertRegex(self, files[1], '.test_%s' % FILE_SUFFIX_REGEX)
495500

496501
class DimensionClosingBug(unittest.TestCase):
497502
"""

0 commit comments

Comments
 (0)
0