commit 30042d475bf084c6723c6217a21598d9247a9c41 Author: Tim Graham Date: Fri Aug 8 10:20:08 2014 -0400 [1.4.x] Fixed #23157 -- Removed O(n) algorithm when uploading duplicate file names. This is a security fix. Disclosure following shortly. Debian note: This patch also includes the minimum backport of additional functionality to django.utils.six to support this patch, but does not change any functionality therein. --- a/django/core/files/storage.py +++ b/django/core/files/storage.py @@ -1,12 +1,12 @@ import os import errno import urlparse -import itertools 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_unicode 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, count.next(), file_ext)) + name = os.path.join(dir_name, "%s_%s%s" % (file_root, get_random_string(7), file_ext)) return name --- a/docs/howto/custom-file-storage.txt +++ b/docs/howto/custom-file-storage.txt @@ -86,5 +86,13 @@ the provided filename into account. The 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.4.14 + + 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. --- a/tests/regressiontests/file_storage/tests.py +++ b/tests/regressiontests/file_storage/tests.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- import os +import re import shutil import sys import tempfile @@ -31,15 +32,15 @@ except ImportError: class FileStorageTests(unittest.TestCase): storage_class = FileSystemStorage - + def setUp(self): self.temp_dir = tempfile.mktemp() os.makedirs(self.temp_dir) self.storage = self.storage_class(location=self.temp_dir) - + def tearDown(self): os.rmdir(self.temp_dir) - + def test_file_access_options(self): """ Standard file access options are available, and work as expected. @@ -53,7 +54,7 @@ class FileStorageTests(unittest.TestCase f = self.storage.open('storage_test', 'r') self.assertEqual(f.read(), 'storage contents') f.close() - + self.storage.delete('storage_test') self.failIf(self.storage.exists('storage_test')) @@ -81,7 +82,7 @@ class CustomStorage(FileSystemStorage): class CustomStorageTests(FileStorageTests): storage_class = CustomStorage - + def test_custom_get_available_name(self): first = self.storage.save('custom_storage', ContentFile('custom contents')) self.assertEqual(first, 'custom_storage') @@ -109,6 +110,9 @@ class SlowFile(ContentFile): time.sleep(1) return super(ContentFile, self).chunks() +FILE_SUFFIX_REGEX = '[A-Za-z0-9]{7}' + + class FileSaveRaceConditionTest(TestCase): def setUp(self): self.storage_dir = tempfile.mkdtemp() @@ -125,10 +129,9 @@ class FileSaveRaceConditionTest(TestCase self.thread.start() name = self.save_file('conflict') self.thread.join() - self.assert_(self.storage.exists('conflict')) - self.assert_(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') + self.assertTrue(re.match('conflict_%s' % FILE_SUFFIX_REGEX, files[1])) class FileStoragePermissions(TestCase): def setUp(self): @@ -165,9 +168,11 @@ class FileStoragePathParsing(TestCase): self.storage.save('dotted.path/test', ContentFile("1")) self.storage.save('dotted.path/test', ContentFile("2")) - self.failIf(os.path.exists(os.path.join(self.storage_dir, 'dotted_.path'))) - self.assert_(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/test'))) - self.assert_(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') + self.assertTrue(re.match('test_%s' % FILE_SUFFIX_REGEX, files[1])) + def test_first_character_dot(self): """ @@ -180,10 +185,13 @@ class FileStoragePathParsing(TestCase): self.assert_(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/.test'))) # Before 2.6, a leading dot was treated as an extension, and so # underscore gets added to beginning instead of end. + files = sorted(os.listdir(os.path.join(self.storage_dir, 'dotted.path'))) + self.assertEqual(files[0], '.test') if sys.version_info < (2, 6): - self.assert_(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/_1.test'))) + self.assertTrue(re.match('_%s.test' % FILE_SUFFIX_REGEX, files[1])) else: - self.assert_(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/.test_1'))) + self.assertTrue(re.match('.test_%s' % FILE_SUFFIX_REGEX, files[1])) + if Image is not None: class DimensionClosingBug(TestCase): --- a/tests/modeltests/files/models.py +++ b/tests/modeltests/files/models.py @@ -90,8 +90,8 @@ True >>> obj2 = Storage() >>> obj2.normal.save('django_test.txt', ContentFile('more content')) ->>> obj2.normal - +>>> obj2.normal # doctest: +ELLIPSIS + >>> obj2.normal.size 12 @@ -100,24 +100,26 @@ True >>> from django.core.cache import cache >>> cache.set('obj1', obj1) >>> cache.set('obj2', obj2) ->>> cache.get('obj2').normal - +>>> cache.get('obj2').normal # doctest: +ELLIPSIS + # Deleting an object deletes the file it uses, if there are no other objects # still using that file. >>> obj2.delete() >>> obj2.normal.save('django_test.txt', ContentFile('more content')) ->>> obj2.normal - +>>> obj2.normal # doctest: +ELLIPSIS + # Multiple files with the same name get _N appended to them. >>> objs = [Storage() for i in range(3)] >>> for o in objs: ... o.normal.save('multiple_files.txt', ContentFile('Same Content')) ->>> [o.normal for o in objs] -[, , ] +>>> [o.normal for o in objs] # doctest: +ELLIPSIS, +NORMALIZE_WHITESPACE +[, + , + ] >>> for o in objs: ... o.delete()