From a77679dfaa963361b6daad6de0d7de1b53d4f104 Mon Sep 17 00:00:00 2001 From: Paul McMillan Date: Sat, 11 Feb 2012 04:18:15 +0000 Subject: [PATCH] Fixes #16827. Adds a length check to CSRF tokens before applying the santizing regex. Thanks to jedie for the report and zsiciarz for the initial patch. git-svn-id: http://code.djangoproject.com/svn/django/trunk@17500 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/middleware/csrf.py | 52 ++++++++++++----------- django/utils/crypto.py | 7 +-- tests/regressiontests/csrf_tests/tests.py | 15 ++++++- 3 files changed, 45 insertions(+), 29 deletions(-) diff --git a/django/middleware/csrf.py b/django/middleware/csrf.py index d46fc7bcd8..1a4e06bbec 100644 --- a/django/middleware/csrf.py +++ b/django/middleware/csrf.py @@ -14,22 +14,16 @@ from django.core.urlresolvers import get_callable from django.utils.cache import patch_vary_headers from django.utils.http import same_origin from django.utils.log import getLogger -from django.utils.crypto import constant_time_compare +from django.utils.crypto import constant_time_compare, get_random_string logger = getLogger('django.request') -# Use the system (hardware-based) random number generator if it exists. -if hasattr(random, 'SystemRandom'): - randrange = random.SystemRandom().randrange -else: - randrange = random.randrange -_MAX_CSRF_KEY = 18446744073709551616L # 2 << 63 - REASON_NO_REFERER = "Referer checking failed - no Referer." REASON_BAD_REFERER = "Referer checking failed - %s does not match %s." REASON_NO_CSRF_COOKIE = "CSRF cookie not set." REASON_BAD_TOKEN = "CSRF token missing or incorrect." +CSRF_KEY_LENGTH = 32 def _get_failure_view(): """ @@ -39,7 +33,7 @@ def _get_failure_view(): def _get_new_csrf_key(): - return hashlib.md5("%s%s" % (randrange(0, _MAX_CSRF_KEY), settings.SECRET_KEY)).hexdigest() + return get_random_string(CSRF_KEY_LENGTH) def get_token(request): @@ -57,14 +51,15 @@ def get_token(request): def _sanitize_token(token): - # Allow only alphanum, and ensure we return a 'str' for the sake of the post - # processing middleware. - token = re.sub('[^a-zA-Z0-9]', '', str(token.decode('ascii', 'ignore'))) + # Allow only alphanum, and ensure we return a 'str' for the sake + # of the post processing middleware. + if len(token) > CSRF_KEY_LENGTH: + return _get_new_csrf_key() + token = re.sub('[^a-zA-Z0-9]+', '', str(token.decode('ascii', 'ignore'))) if token == "": # In case the cookie has been truncated to nothing at some point. return _get_new_csrf_key() - else: - return token + return token class CsrfViewMiddleware(object): @@ -94,12 +89,14 @@ class CsrfViewMiddleware(object): return None try: - csrf_token = _sanitize_token(request.COOKIES[settings.CSRF_COOKIE_NAME]) + csrf_token = _sanitize_token( + request.COOKIES[settings.CSRF_COOKIE_NAME]) # Use same token next time request.META['CSRF_COOKIE'] = csrf_token except KeyError: csrf_token = None - # Generate token and store it in the request, so it's available to the view. + # Generate token and store it in the request, so it's + # available to the view. request.META["CSRF_COOKIE"] = _get_new_csrf_key() # Wait until request.META["CSRF_COOKIE"] has been manipulated before @@ -107,13 +104,14 @@ class CsrfViewMiddleware(object): if getattr(callback, 'csrf_exempt', False): return None - # Assume that anything not defined as 'safe' by RC2616 needs protection. + # Assume that anything not defined as 'safe' by RC2616 needs protection if request.method not in ('GET', 'HEAD', 'OPTIONS', 'TRACE'): if getattr(request, '_dont_enforce_csrf_checks', False): - # Mechanism to turn off CSRF checks for test suite. It comes after - # the creation of CSRF cookies, so that everything else continues to - # work exactly the same (e.g. cookies are sent etc), but before the - # any branches that call reject() + # Mechanism to turn off CSRF checks for test suite. + # It comes after the creation of CSRF cookies, so that + # everything else continues to work exactly the same + # (e.g. cookies are sent etc), but before the any + # branches that call reject() return self._accept(request) if request.is_secure(): @@ -134,7 +132,8 @@ class CsrfViewMiddleware(object): # we can use strict Referer checking. referer = request.META.get('HTTP_REFERER') if referer is None: - logger.warning('Forbidden (%s): %s', REASON_NO_REFERER, request.path, + logger.warning('Forbidden (%s): %s', + REASON_NO_REFERER, request.path, extra={ 'status_code': 403, 'request': request, @@ -158,7 +157,8 @@ class CsrfViewMiddleware(object): # No CSRF cookie. For POST requests, we insist on a CSRF cookie, # and in this way we can avoid all CSRF attacks, including login # CSRF. - logger.warning('Forbidden (%s): %s', REASON_NO_CSRF_COOKIE, request.path, + logger.warning('Forbidden (%s): %s', + REASON_NO_CSRF_COOKIE, request.path, extra={ 'status_code': 403, 'request': request, @@ -177,7 +177,8 @@ class CsrfViewMiddleware(object): request_csrf_token = request.META.get('HTTP_X_CSRFTOKEN', '') if not constant_time_compare(request_csrf_token, csrf_token): - logger.warning('Forbidden (%s): %s', REASON_BAD_TOKEN, request.path, + logger.warning('Forbidden (%s): %s', + REASON_BAD_TOKEN, request.path, extra={ 'status_code': 403, 'request': request, @@ -200,7 +201,8 @@ class CsrfViewMiddleware(object): if not request.META.get("CSRF_COOKIE_USED", False): return response - # Set the CSRF cookie even if it's already set, so we renew the expiry timer. + # Set the CSRF cookie even if it's already set, so we renew + # the expiry timer. response.set_cookie(settings.CSRF_COOKIE_NAME, request.META["CSRF_COOKIE"], max_age = 60 * 60 * 24 * 7 * 52, diff --git a/django/utils/crypto.py b/django/utils/crypto.py index f3a0675cc6..4fe11f428c 100644 --- a/django/utils/crypto.py +++ b/django/utils/crypto.py @@ -36,10 +36,11 @@ def salted_hmac(key_salt, value, secret=None): return hmac.new(key, msg=value, digestmod=hashlib.sha1) -def get_random_string(length=12, allowed_chars='abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789'): +def get_random_string(length=12, + allowed_chars='abcdefghijklmnopqrstuvwxyz' + 'ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789'): """ - Returns a random string of length characters from the set of a-z, A-Z, 0-9 - for use as a salt. + Returns a random string of length characters from the set of a-z, A-Z, 0-9. The default length of 12 with the a-z, A-Z, 0-9 character set returns a 71-bit salt. log_2((26+26+10)^12) =~ 71 bits diff --git a/tests/regressiontests/csrf_tests/tests.py b/tests/regressiontests/csrf_tests/tests.py index 6e6c87a112..71400ead89 100644 --- a/tests/regressiontests/csrf_tests/tests.py +++ b/tests/regressiontests/csrf_tests/tests.py @@ -4,7 +4,7 @@ from __future__ import with_statement from django.conf import settings from django.core.context_processors import csrf from django.http import HttpRequest, HttpResponse -from django.middleware.csrf import CsrfViewMiddleware +from django.middleware.csrf import CsrfViewMiddleware, CSRF_KEY_LENGTH from django.template import RequestContext, Template from django.test import TestCase from django.views.decorators.csrf import csrf_exempt, requires_csrf_token, ensure_csrf_cookie @@ -77,6 +77,19 @@ class CsrfViewMiddlewareTest(TestCase): def _check_token_present(self, response, csrf_id=None): self.assertContains(response, "name='csrfmiddlewaretoken' value='%s'" % (csrf_id or self._csrf_id)) + def test_process_view_token_too_long(self): + """ + Check that if the token is longer than expected, it is ignored and + a new token is created. + """ + req = self._get_GET_no_csrf_cookie_request() + req.COOKIES[settings.CSRF_COOKIE_NAME] = 'x' * 10000000 + CsrfViewMiddleware().process_view(req, token_view, (), {}) + resp = token_view(req) + resp2 = CsrfViewMiddleware().process_response(req, resp) + csrf_cookie = resp2.cookies.get(settings.CSRF_COOKIE_NAME, False) + self.assertEqual(len(csrf_cookie.value), CSRF_KEY_LENGTH) + def test_process_response_get_token_used(self): """ When get_token is used, check that the cookie is created and headers