Commit 26cd48e1 authored by Tim Graham's avatar Tim Graham
Browse files

[1.5.x] Fixed #23157 -- Removed O(n) algorithm when uploading duplicate file names.

This is a security fix. Disclosure following shortly.
parent 45ac9d4f
Loading
Loading
Loading
Loading
+5 −6
Original line number Diff line number Diff line
@@ -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

+11 −2
Original line number Diff line number Diff line
@@ -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.
+11 −0
Original line number Diff line number Diff line
@@ -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)

+20 −0
Original line number Diff line number Diff line
@@ -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.).
+20 −0
Original line number Diff line number Diff line
@@ -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.).
Loading