From 5112e65ef2df1dbb95ff83026b6a962fb2688661 Mon Sep 17 00:00:00 2001 From: Shai Berger Date: Sat, 7 Nov 2015 18:35:45 +0200 Subject: Fixed #20869 -- made CSRF tokens change every request by salt-encrypting them Note that the cookie is not changed every request, just the token retrieved by the `get_token()` method (used also by the `{% csrf_token %}` tag). While at it, made token validation strict: Where, before, any length was accepted and non-ASCII chars were ignored, we now treat anything other than `[A-Za-z0-9]{64}` as invalid (except for 32-char tokens, which, for backwards-compatibility, are accepted and replaced by 64-char ones). Thanks Trac user patrys for reporting, github user adambrenecki for initial patch, Tim Graham for help, and Curtis Maloney, Collin Anderson, Florian Apolloner, Markus Holtermann & Jon Dufresne for reviews. --- tests/csrf_tests/test_context_processor.py | 8 +-- tests/csrf_tests/tests.py | 100 ++++++++++++++++++++++++++--- tests/template_backends/test_dummy.py | 15 +++-- 3 files changed, 104 insertions(+), 19 deletions(-) (limited to 'tests') diff --git a/tests/csrf_tests/test_context_processor.py b/tests/csrf_tests/test_context_processor.py index 270b3e4771..5db0116db0 100644 --- a/tests/csrf_tests/test_context_processor.py +++ b/tests/csrf_tests/test_context_processor.py @@ -1,6 +1,5 @@ -import json - from django.http import HttpRequest +from django.middleware.csrf import _compare_salted_tokens as equivalent_tokens from django.template.context_processors import csrf from django.test import SimpleTestCase from django.utils.encoding import force_text @@ -10,6 +9,7 @@ class TestContextProcessor(SimpleTestCase): def test_force_text_on_token(self): request = HttpRequest() - request.META['CSRF_COOKIE'] = 'test-token' + test_token = '1bcdefghij2bcdefghij3bcdefghij4bcdefghij5bcdefghij6bcdefghijABCD' + request.META['CSRF_COOKIE'] = test_token token = csrf(request).get('csrf_token') - self.assertEqual(json.dumps(force_text(token)), '"test-token"') + self.assertTrue(equivalent_tokens(force_text(token), test_token)) diff --git a/tests/csrf_tests/tests.py b/tests/csrf_tests/tests.py index 54570dca42..5ed7c9dc3d 100644 --- a/tests/csrf_tests/tests.py +++ b/tests/csrf_tests/tests.py @@ -2,15 +2,20 @@ from __future__ import unicode_literals import logging +import re +import warnings from django.conf import settings from django.http import HttpRequest, HttpResponse from django.middleware.csrf import ( - CSRF_KEY_LENGTH, CsrfViewMiddleware, get_token, + CSRF_TOKEN_LENGTH, CsrfViewMiddleware, + _compare_salted_tokens as equivalent_tokens, get_token, ) from django.template import RequestContext, Template from django.template.context_processors import csrf from django.test import SimpleTestCase, override_settings +from django.utils.encoding import force_bytes +from django.utils.six import text_type from django.views.decorators.csrf import ( csrf_exempt, ensure_csrf_cookie, requires_csrf_token, ) @@ -57,10 +62,7 @@ class TestingHttpRequest(HttpRequest): class CsrfViewMiddlewareTest(SimpleTestCase): - # The csrf token is potentially from an untrusted source, so could have - # characters that need dealing with. - _csrf_id_cookie = b"<1>\xc2\xa1" - _csrf_id = "1" + _csrf_id = _csrf_id_cookie = '1bcdefghij2bcdefghij3bcdefghij4bcdefghij5bcdefghij6bcdefghijABCD' def _get_GET_no_csrf_cookie_request(self): return TestingHttpRequest() @@ -85,8 +87,24 @@ class CsrfViewMiddlewareTest(SimpleTestCase): req.POST['csrfmiddlewaretoken'] = self._csrf_id return req + def _get_POST_bare_secret_csrf_cookie_request(self): + req = self._get_POST_no_csrf_cookie_request() + req.COOKIES[settings.CSRF_COOKIE_NAME] = self._csrf_id_cookie[:32] + return req + + def _get_POST_bare_secret_csrf_cookie_request_with_token(self): + req = self._get_POST_bare_secret_csrf_cookie_request() + req.POST['csrfmiddlewaretoken'] = self._csrf_id_cookie[:32] + return req + def _check_token_present(self, response, csrf_id=None): - self.assertContains(response, "name='csrfmiddlewaretoken' value='%s'" % (csrf_id or self._csrf_id)) + text = text_type(response.content, response.charset) + match = re.search("name='csrfmiddlewaretoken' value='(.*?)'", text) + csrf_token = csrf_id or self._csrf_id + self.assertTrue( + match and equivalent_tokens(csrf_token, match.group(1)), + "Could not find csrfmiddlewaretoken to match %s" % csrf_token + ) def test_process_view_token_too_long(self): """ @@ -94,12 +112,45 @@ class CsrfViewMiddlewareTest(SimpleTestCase): created. """ req = self._get_GET_no_csrf_cookie_request() - req.COOKIES[settings.CSRF_COOKIE_NAME] = 'x' * 10000000 + req.COOKIES[settings.CSRF_COOKIE_NAME] = 'x' * 100000 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) + self.assertEqual(len(csrf_cookie.value), CSRF_TOKEN_LENGTH) + + def test_process_view_token_invalid_chars(self): + """ + If the token contains non-alphanumeric characters, it is ignored and a + new token is created. + """ + token = ('!@#' + self._csrf_id)[:CSRF_TOKEN_LENGTH] + req = self._get_GET_no_csrf_cookie_request() + req.COOKIES[settings.CSRF_COOKIE_NAME] = token + 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_TOKEN_LENGTH) + self.assertNotEqual(csrf_cookie.value, token) + + def test_process_view_token_invalid_bytes(self): + """ + If the token contains improperly encoded unicode, it is ignored and a + new token is created. + """ + token = (b"<1>\xc2\xa1" + force_bytes(self._csrf_id, 'ascii'))[:CSRF_TOKEN_LENGTH] + req = self._get_GET_no_csrf_cookie_request() + req.COOKIES[settings.CSRF_COOKIE_NAME] = token + # We expect a UnicodeWarning here, because we used broken utf-8 on purpose + with warnings.catch_warnings(): + warnings.filterwarnings("ignore", category=UnicodeWarning) + 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_TOKEN_LENGTH) + self.assertNotEqual(csrf_cookie.value, token) def test_process_response_get_token_used(self): """ @@ -295,6 +346,37 @@ class CsrfViewMiddlewareTest(SimpleTestCase): csrf_cookie = resp2.cookies[settings.CSRF_COOKIE_NAME] self._check_token_present(resp, csrf_id=csrf_cookie.value) + def test_cookie_not_reset_on_accepted_request(self): + """ + The csrf token used in posts is changed on every request (although + stays equivalent). The csrf cookie should not change on accepted + requests. If it appears in the response, it should keep its value. + """ + req = self._get_POST_request_with_token() + CsrfViewMiddleware().process_view(req, token_view, (), {}) + resp = token_view(req) + resp = CsrfViewMiddleware().process_response(req, resp) + csrf_cookie = resp.cookies.get(settings.CSRF_COOKIE_NAME, None) + if csrf_cookie: + self.assertEqual( + csrf_cookie.value, self._csrf_id_cookie, + "CSRF cookie was changed on an accepted request" + ) + + def test_bare_secret_accepted_and_replaced(self): + """ + The csrf token is reset from a bare secret. + """ + req = self._get_POST_bare_secret_csrf_cookie_request_with_token() + req2 = CsrfViewMiddleware().process_view(req, token_view, (), {}) + self.assertIsNone(req2) + resp = token_view(req) + resp = CsrfViewMiddleware().process_response(req, resp) + self.assertIn(settings.CSRF_COOKIE_NAME, resp.cookies, "Cookie was not reset from bare secret") + csrf_cookie = resp.cookies[settings.CSRF_COOKIE_NAME] + self.assertEqual(len(csrf_cookie.value), CSRF_TOKEN_LENGTH) + self._check_token_present(resp, csrf_id=csrf_cookie.value) + @override_settings(DEBUG=True) def test_https_bad_referer(self): """ @@ -592,7 +674,7 @@ class CsrfViewMiddlewareTest(SimpleTestCase): POST = property(_get_post, _set_post) - token = 'ABC' + token = ('ABC' + self._csrf_id)[:CSRF_TOKEN_LENGTH] req = CsrfPostRequest(token, raise_error=False) resp = CsrfViewMiddleware().process_view(req, post_form_view, (), {}) diff --git a/tests/template_backends/test_dummy.py b/tests/template_backends/test_dummy.py index e6e60e71a2..83b42c7eb4 100644 --- a/tests/template_backends/test_dummy.py +++ b/tests/template_backends/test_dummy.py @@ -2,9 +2,13 @@ from __future__ import unicode_literals +import re + from django.forms import CharField, Form, Media from django.http import HttpRequest -from django.middleware.csrf import CsrfViewMiddleware, get_token +from django.middleware.csrf import ( + CsrfViewMiddleware, _compare_salted_tokens as equivalent_tokens, get_token, +) from django.template import TemplateDoesNotExist, TemplateSyntaxError from django.template.backends.dummy import TemplateStrings from django.test import SimpleTestCase @@ -81,11 +85,10 @@ class TemplateStringsTests(SimpleTestCase): template = self.engine.get_template('template_backends/csrf.html') content = template.render(request=request) - expected = ( - ''.format(get_token(request))) - - self.assertHTMLEqual(content, expected) + expected = '' + match = re.match(expected, content) or re.match(expected.replace('"', "'"), content) + self.assertTrue(match, "hidden csrftoken field not found in output") + self.assertTrue(equivalent_tokens(match.group(1), get_token(request))) def test_no_directory_traversal(self): with self.assertRaises(TemplateDoesNotExist): -- cgit v1.3