Login
[x]
Log in using an account from:
Fedora Account System
Red Hat Associate
Red Hat Customer
Or login using a Red Hat Bugzilla account
Forgot Password
Login:
Hide Forgot
Create an Account
Red Hat Bugzilla – Attachment 926639 Details for
Bug 1129952
CVE-2014-0481 Django: file upload denial of service
[?]
New
Simple Search
Advanced Search
My Links
Browse
Requests
Reports
Current State
Search
Tabular reports
Graphical reports
Duplicates
Other Reports
User Changes
Plotly Reports
Bug Status
Bug Severity
Non-Defaults
|
Product Dashboard
Help
Page Help!
Bug Writing Guidelines
What's new
Browser Support Policy
5.0.4.rh83 Release notes
FAQ
Guides index
User guide
Web Services
Contact
Legal
This site requires JavaScript to be enabled to function correctly, please enable it.
[patch]
1.5 patch
file-upload-1.5.diff (text/plain), 12.04 KB, created by
Murray McAllister
on 2014-08-14 04:46:17 UTC
(
hide
)
Description:
1.5 patch
Filename:
MIME Type:
Creator:
Murray McAllister
Created:
2014-08-14 04:46:17 UTC
Size:
12.04 KB
patch
obsolete
>commit da251da43d6e90df9b8a433770272c13aad68d44 >Author: Tim Graham <timograham@gmail.com> >Date: Fri Aug 8 10:20:08 2014 -0400 > > [1.5.x] Fixed #23157 -- Removed O(n) algorithm when uploading duplicate file names. > > This is a security fix. Disclosure following shortly. > >diff --git a/django/core/files/storage.py b/django/core/files/storage.py >index 650373f..fdf8a5f 100644 >--- a/django/core/files/storage.py >+++ b/django/core/files/storage.py >@@ -4,13 +4,13 @@ try: > from urllib.parse import urljoin > except ImportError: # Python 2 > from urlparse import urljoin >-import itertools > from datetime import datetime > > from django.conf import settings > from django.core.exceptions import ImproperlyConfigured, SuspiciousOperation > from django.core.files import locks, File > from django.core.files.move import file_move_safe >+from django.utils.crypto import get_random_string > from django.utils.encoding import force_text, filepath_to_uri > from django.utils.functional import LazyObject > from django.utils.importlib import import_module >@@ -66,13 +66,12 @@ class Storage(object): > """ > dir_name, file_name = os.path.split(name) > file_root, file_ext = os.path.splitext(file_name) >- # If the filename already exists, add an underscore and a number (before >- # the file extension, if one exists) to the filename until the generated >- # filename doesn't exist. >- count = itertools.count(1) >+ # If the filename already exists, add an underscore and a random 7 >+ # character alphanumeric string (before the file extension, if one >+ # exists) to the filename until the generated filename doesn't exist. > while self.exists(name): > # file_ext includes the dot. >- name = os.path.join(dir_name, "%s_%s%s" % (file_root, next(count), file_ext)) >+ name = os.path.join(dir_name, "%s_%s%s" % (file_root, get_random_string(7), file_ext)) > > return name > >diff --git a/docs/howto/custom-file-storage.txt b/docs/howto/custom-file-storage.txt >index 090cc08..509ab2e 100644 >--- a/docs/howto/custom-file-storage.txt >+++ b/docs/howto/custom-file-storage.txt >@@ -83,5 +83,14 @@ the provided filename into account. The ``name`` argument passed to this method > will have already cleaned to a filename valid for the storage system, according > to the ``get_valid_name()`` method described above. > >-The code provided on ``Storage`` simply appends ``"_1"``, ``"_2"``, etc. to the >-filename until it finds one that's available in the destination directory. >+.. versionchanged:: 1.5.9 >+ >+ If a file with ``name`` already exists, an underscore plus a random 7 >+ character alphanumeric string is appended to the filename before the >+ extension. >+ >+ Previously, an underscore followed by a number (e.g. ``"_1"``, ``"_2"``, >+ etc.) was appended to the filename until an avaible name in the destination >+ directory was found. A malicious user could exploit this deterministic >+ algorithm to create a denial-of-service attack. This change was also made >+ in Django 1.4.14. >diff --git a/docs/ref/files/storage.txt b/docs/ref/files/storage.txt >index 1216e85..fd90e31 100644 >--- a/docs/ref/files/storage.txt >+++ b/docs/ref/files/storage.txt >@@ -81,6 +81,17 @@ The Storage Class > available for new content to be written to on the target storage > system. > >+ .. versionchanged:: 1.5.9 >+ >+ If a file with ``name`` already exists, an underscore plus a random 7 >+ character alphanumeric string is appended to the filename before the >+ extension. >+ >+ Previously, an underscore followed by a number (e.g. ``"_1"``, ``"_2"``, >+ etc.) was appended to the filename until an avaible name in the >+ destination directory was found. A malicious user could exploit this >+ deterministic algorithm to create a denial-of-service attack. This >+ change was also made in Django 1.4.14. > > .. method:: get_valid_name(name) > >diff --git a/docs/releases/1.4.14.txt b/docs/releases/1.4.14.txt >index 28390c9..6c140ee 100644 >--- a/docs/releases/1.4.14.txt >+++ b/docs/releases/1.4.14.txt >@@ -18,3 +18,23 @@ To remedy this, URL reversing now ensures that no URL starts with two slashes > (//), replacing the second slash with its URL encoded counterpart (%2F). This > approach ensures that semantics stay the same, while making the URL relative to > the domain and not to the scheme. >+ >+File upload denial-of-service >+============================= >+ >+Before this release, Django's file upload handing in its default configuration >+may degrade to producing a huge number of ``os.stat()`` system calls when a >+duplicate filename is uploaded. Since ``stat()`` may invoke IO, this may produce >+a huge data-dependent slowdown that slowly worsens over time. The net result is >+that given enough time, a user with the ability to upload files can cause poor >+performance in the upload handler, eventually causing it to become very slow >+simply by uploading 0-byte files. At this point, even a slow network connection >+and few HTTP requests would be all that is necessary to make a site unavailable. >+ >+We've remedied the issue by changing the algorithm for generating file names >+if a file with the uploaded name already exists. >+:meth:`Storage.get_available_name() >+<django.core.files.storage.Storage.get_available_name>` now appends an >+underscore plus a random 7 character alphanumeric string (e.g. ``"_x3a1gho"``), >+rather than iterating through an underscore followed by a number (e.g. ``"_1"``, >+``"_2"``, etc.). >diff --git a/docs/releases/1.5.9.txt b/docs/releases/1.5.9.txt >index 12b5b5f..232cbb0 100644 >--- a/docs/releases/1.5.9.txt >+++ b/docs/releases/1.5.9.txt >@@ -18,3 +18,23 @@ To remedy this, URL reversing now ensures that no URL starts with two slashes > (//), replacing the second slash with its URL encoded counterpart (%2F). This > approach ensures that semantics stay the same, while making the URL relative to > the domain and not to the scheme. >+ >+File upload denial-of-service >+============================= >+ >+Before this release, Django's file upload handing in its default configuration >+may degrade to producing a huge number of ``os.stat()`` system calls when a >+duplicate filename is uploaded. Since ``stat()`` may invoke IO, this may produce >+a huge data-dependent slowdown that slowly worsens over time. The net result is >+that given enough time, a user with the ability to upload files can cause poor >+performance in the upload handler, eventually causing it to become very slow >+simply by uploading 0-byte files. At this point, even a slow network connection >+and few HTTP requests would be all that is necessary to make a site unavailable. >+ >+We've remedied the issue by changing the algorithm for generating file names >+if a file with the uploaded name already exists. >+:meth:`Storage.get_available_name() >+<django.core.files.storage.Storage.get_available_name>` now appends an >+underscore plus a random 7 character alphanumeric string (e.g. ``"_x3a1gho"``), >+rather than iterating through an underscore followed by a number (e.g. ``"_1"``, >+``"_2"``, etc.). >diff --git a/tests/modeltests/files/tests.py b/tests/modeltests/files/tests.py >index 50d1915..e888fbf 100644 >--- a/tests/modeltests/files/tests.py >+++ b/tests/modeltests/files/tests.py >@@ -9,11 +9,14 @@ from django.core.files import File > from django.core.files.base import ContentFile > from django.core.files.uploadedfile import SimpleUploadedFile > from django.test import TestCase >-from django.utils import unittest >+from django.utils import six, unittest > > from .models import Storage, temp_storage, temp_storage_location > > >+FILE_SUFFIX_REGEX = '[A-Za-z0-9]{7}' >+ >+ > class FileStorageTests(TestCase): > def tearDown(self): > shutil.rmtree(temp_storage_location) >@@ -59,27 +62,28 @@ class FileStorageTests(TestCase): > # Save another file with the same name. > obj2 = Storage() > obj2.normal.save("django_test.txt", ContentFile("more content")) >- self.assertEqual(obj2.normal.name, "tests/django_test_1.txt") >+ obj2_name = obj2.normal.name >+ six.assertRegex(self, obj2_name, "tests/django_test_%s.txt" % FILE_SUFFIX_REGEX) > self.assertEqual(obj2.normal.size, 12) > > # Push the objects into the cache to make sure they pickle properly > cache.set("obj1", obj1) > cache.set("obj2", obj2) >- self.assertEqual(cache.get("obj2").normal.name, "tests/django_test_1.txt") >+ six.assertRegex(self, cache.get("obj2").normal.name, "tests/django_test_%s.txt" % FILE_SUFFIX_REGEX) > > # Deleting an object does not delete the file it uses. > obj2.delete() > obj2.normal.save("django_test.txt", ContentFile("more content")) >- self.assertEqual(obj2.normal.name, "tests/django_test_2.txt") >+ self.assertNotEqual(obj2_name, obj2.normal.name) >+ six.assertRegex(self, obj2.normal.name, "tests/django_test_%s.txt" % FILE_SUFFIX_REGEX) > > # Multiple files with the same name get _N appended to them. >- objs = [Storage() for i in range(3)] >+ objs = [Storage() for i in range(2)] > for o in objs: > o.normal.save("multiple_files.txt", ContentFile("Same Content")) >- self.assertEqual( >- [o.normal.name for o in objs], >- ["tests/multiple_files.txt", "tests/multiple_files_1.txt", "tests/multiple_files_2.txt"] >- ) >+ names = [o.normal.name for o in objs] >+ self.assertEqual(names[0], "tests/multiple_files.txt") >+ six.assertRegex(self, names[1], "tests/multiple_files_%s.txt" % FILE_SUFFIX_REGEX) > for o in objs: > o.delete() > >diff --git a/tests/regressiontests/file_storage/tests.py b/tests/regressiontests/file_storage/tests.py >index b6d3e1f..6960b1f 100644 >--- a/tests/regressiontests/file_storage/tests.py >+++ b/tests/regressiontests/file_storage/tests.py >@@ -40,6 +40,9 @@ except ImportError: > Image = None > > >+FILE_SUFFIX_REGEX = '[A-Za-z0-9]{7}' >+ >+ > class GetStorageClassTests(SimpleTestCase): > > def test_get_filesystem_storage(self): >@@ -431,10 +434,9 @@ class FileSaveRaceConditionTest(unittest.TestCase): > self.thread.start() > name = self.save_file('conflict') > self.thread.join() >- self.assertTrue(self.storage.exists('conflict')) >- self.assertTrue(self.storage.exists('conflict_1')) >- self.storage.delete('conflict') >- self.storage.delete('conflict_1') >+ files = sorted(os.listdir(self.storage_dir)) >+ self.assertEqual(files[0], 'conflict') >+ six.assertRegex(self, files[1], 'conflict_%s' % FILE_SUFFIX_REGEX) > > @unittest.skipIf(sys.platform.startswith('win'), "Windows only partially supports umasks and chmod.") > class FileStoragePermissions(unittest.TestCase): >@@ -478,9 +480,10 @@ class FileStoragePathParsing(unittest.TestCase): > self.storage.save('dotted.path/test', ContentFile("1")) > self.storage.save('dotted.path/test', ContentFile("2")) > >+ files = sorted(os.listdir(os.path.join(self.storage_dir, 'dotted.path'))) > self.assertFalse(os.path.exists(os.path.join(self.storage_dir, 'dotted_.path'))) >- self.assertTrue(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/test'))) >- self.assertTrue(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/test_1'))) >+ self.assertEqual(files[0], 'test') >+ six.assertRegex(self, files[1], 'test_%s' % FILE_SUFFIX_REGEX) > > def test_first_character_dot(self): > """ >@@ -490,8 +493,10 @@ class FileStoragePathParsing(unittest.TestCase): > self.storage.save('dotted.path/.test', ContentFile("1")) > self.storage.save('dotted.path/.test', ContentFile("2")) > >- self.assertTrue(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/.test'))) >- self.assertTrue(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/.test_1'))) >+ files = sorted(os.listdir(os.path.join(self.storage_dir, 'dotted.path'))) >+ self.assertFalse(os.path.exists(os.path.join(self.storage_dir, 'dotted_.path'))) >+ self.assertEqual(files[0], '.test') >+ six.assertRegex(self, files[1], '.test_%s' % FILE_SUFFIX_REGEX) > > class DimensionClosingBug(unittest.TestCase): > """
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
Attachments on
bug 1129952
:
926638
| 926639 |
926640