From ede59ef6f39ff8a6443c2b24df0208ef6ec41ee0 Mon Sep 17 00:00:00 2001 From: Romain Garrigues Date: Fri, 13 Jan 2017 14:17:54 +0000 Subject: [PATCH] Fixed #27518 -- Prevented possibie password reset token leak via HTTP Referer header. Thanks Florian Apolloner for contributing to this patch and Collin Anderson, Markus Holtermann, and Tim Graham for review. --- AUTHORS | 1 + django/contrib/auth/tokens.py | 2 ++ django/contrib/auth/views.py | 29 +++++++++++++++++---- docs/releases/1.11.txt | 8 ++++++ tests/auth_tests/client.py | 41 ++++++++++++++++++++++++++++++ tests/auth_tests/test_templates.py | 13 +++++++--- tests/auth_tests/test_tokens.py | 7 +++++ tests/auth_tests/test_views.py | 31 ++++++++++++++++++++-- 8 files changed, 122 insertions(+), 10 deletions(-) create mode 100644 tests/auth_tests/client.py diff --git a/AUTHORS b/AUTHORS index c356dce59f..7365ff8788 100644 --- a/AUTHORS +++ b/AUTHORS @@ -662,6 +662,7 @@ answer newbie questions, and generally made Django that much better: Robert Wittams Rob Hudson Robin Munn + Romain Garrigues Ronny Haryanto Ross Poulton Rozza diff --git a/django/contrib/auth/tokens.py b/django/contrib/auth/tokens.py index 46b8467476..6cf694cebb 100644 --- a/django/contrib/auth/tokens.py +++ b/django/contrib/auth/tokens.py @@ -24,6 +24,8 @@ class PasswordResetTokenGenerator(object): """ Check that a password reset token is correct for a given user. """ + if not (user and token): + return False # Parse the token try: ts_b36, hash = token.split("-") diff --git a/django/contrib/auth/views.py b/django/contrib/auth/views.py index 8edf02a7d1..be156286c6 100644 --- a/django/contrib/auth/views.py +++ b/django/contrib/auth/views.py @@ -425,6 +425,10 @@ class PasswordResetView(PasswordContextMixin, FormView): return super(PasswordResetView, self).form_valid(form) +INTERNAL_RESET_URL_TOKEN = 'set-password' +INTERNAL_RESET_SESSION_TOKEN = '_password_reset_token' + + class PasswordResetDoneView(PasswordContextMixin, TemplateView): template_name = 'registration/password_reset_done.html' title = _('Password reset sent') @@ -446,12 +450,26 @@ class PasswordResetConfirmView(PasswordContextMixin, FormView): self.validlink = False self.user = self.get_user(kwargs['uidb64']) - if self.user is not None and self.token_generator.check_token(self.user, kwargs['token']): - self.validlink = True - else: - return self.render_to_response(self.get_context_data()) + if self.user is not None: + token = kwargs['token'] + if token == INTERNAL_RESET_URL_TOKEN: + session_token = self.request.session.get(INTERNAL_RESET_SESSION_TOKEN) + if self.token_generator.check_token(self.user, session_token): + # If the token is valid, display the password reset form. + self.validlink = True + return super(PasswordResetConfirmView, self).dispatch(*args, **kwargs) + else: + if self.token_generator.check_token(self.user, token): + # Store the token in the session and redirect to the + # password reset form at a URL without the token. That + # avoids the possibility of leaking the token in the + # HTTP Referer header. + self.request.session[INTERNAL_RESET_SESSION_TOKEN] = token + redirect_url = self.request.path.replace(token, INTERNAL_RESET_URL_TOKEN) + return HttpResponseRedirect(redirect_url) - return super(PasswordResetConfirmView, self).dispatch(*args, **kwargs) + # Display the "Password reset unsuccessful" page. + return self.render_to_response(self.get_context_data()) def get_user(self, uidb64): try: @@ -471,6 +489,7 @@ class PasswordResetConfirmView(PasswordContextMixin, FormView): user = form.save() if self.post_reset_login: auth_login(self.request, user) + del self.request.session[INTERNAL_RESET_SESSION_TOKEN] return super(PasswordResetConfirmView, self).form_valid(form) def get_context_data(self, **kwargs): diff --git a/docs/releases/1.11.txt b/docs/releases/1.11.txt index 9bb9a2e903..c8f3e92f21 100644 --- a/docs/releases/1.11.txt +++ b/docs/releases/1.11.txt @@ -116,6 +116,14 @@ Minor features :class:`~django.contrib.auth.views.PasswordResetConfirmView` allows automatically logging in a user after a successful password reset. +* To avoid the possibility of leaking a password reset token via the HTTP + Referer header (for example, if the reset page includes a reference to CSS or + JavaScript hosted on another domain), the + :class:`~django.contrib.auth.views.PasswordResetConfirmView` (but not the + deprecated ``password_reset_confirm()`` function-based view) stores the token + in a session and redirects to itself to present the password change form to + the user without the token in the URL. + * :func:`~django.contrib.auth.update_session_auth_hash` now rotates the session key to allow a password change to invalidate stolen session cookies. diff --git a/tests/auth_tests/client.py b/tests/auth_tests/client.py new file mode 100644 index 0000000000..004101108b --- /dev/null +++ b/tests/auth_tests/client.py @@ -0,0 +1,41 @@ +import re + +from django.contrib.auth.views import ( + INTERNAL_RESET_SESSION_TOKEN, INTERNAL_RESET_URL_TOKEN, +) +from django.test import Client + + +def extract_token_from_url(url): + token_search = re.search(r'/reset/.*/(.+?)/', url) + if token_search: + return token_search.group(1) + + +class PasswordResetConfirmClient(Client): + """ + This client eases testing the password reset flow by emulating the + PasswordResetConfirmView's redirect and saving of the reset token in the + user's session. This request puts 'my-token' in the session and redirects + to '/reset/bla/set-password/': + + >>> client = PasswordResetConfirmClient() + >>> client.get('/reset/bla/my-token/') + """ + def _get_password_reset_confirm_redirect_url(self, url): + token = extract_token_from_url(url) + if not token: + return url + # Add the token to the session + session = self.session + session[INTERNAL_RESET_SESSION_TOKEN] = token + session.save() + return url.replace(token, INTERNAL_RESET_URL_TOKEN) + + def get(self, path, *args, **kwargs): + redirect_url = self._get_password_reset_confirm_redirect_url(path) + return super(PasswordResetConfirmClient, self).get(redirect_url, *args, **kwargs) + + def post(self, path, *args, **kwargs): + redirect_url = self._get_password_reset_confirm_redirect_url(path) + return super(PasswordResetConfirmClient, self).post(redirect_url, *args, **kwargs) diff --git a/tests/auth_tests/test_templates.py b/tests/auth_tests/test_templates.py index 9414f8b299..a1d14c9774 100644 --- a/tests/auth_tests/test_templates.py +++ b/tests/auth_tests/test_templates.py @@ -3,12 +3,15 @@ from django.contrib.auth.models import User from django.contrib.auth.tokens import PasswordResetTokenGenerator from django.contrib.auth.views import ( PasswordChangeDoneView, PasswordChangeView, PasswordResetCompleteView, - PasswordResetConfirmView, PasswordResetDoneView, PasswordResetView, + PasswordResetDoneView, PasswordResetView, ) from django.test import RequestFactory, TestCase, override_settings +from django.urls import reverse from django.utils.encoding import force_bytes, force_text from django.utils.http import urlsafe_base64_encode +from .client import PasswordResetConfirmClient + @override_settings(ROOT_URLCONF='auth_tests.urls') class AuthTemplateTests(TestCase): @@ -34,16 +37,20 @@ class AuthTemplateTests(TestCase): def test_PasswordResetConfirmView_invalid_token(self): # PasswordResetConfirmView invalid token - response = PasswordResetConfirmView.as_view(success_url='dummy/')(self.request, uidb64='Bad', token='Bad') + client = PasswordResetConfirmClient() + url = reverse('password_reset_confirm', kwargs={'uidb64': 'Bad', 'token': 'Bad-Token'}) + response = client.get(url) self.assertContains(response, 'Password reset unsuccessful') self.assertContains(response, '

Password reset unsuccessful

') def test_PasswordResetConfirmView_valid_token(self): # PasswordResetConfirmView valid token + client = PasswordResetConfirmClient() default_token_generator = PasswordResetTokenGenerator() token = default_token_generator.make_token(self.user) uidb64 = force_text(urlsafe_base64_encode(force_bytes(self.user.pk))) - response = PasswordResetConfirmView.as_view(success_url='dummy/')(self.request, uidb64=uidb64, token=token) + url = reverse('password_reset_confirm', kwargs={'uidb64': uidb64, 'token': token}) + response = client.get(url) self.assertContains(response, 'Enter new password') self.assertContains(response, '

Enter new password

') diff --git a/tests/auth_tests/test_tokens.py b/tests/auth_tests/test_tokens.py index 99f9741a0a..7ff3f15f3d 100644 --- a/tests/auth_tests/test_tokens.py +++ b/tests/auth_tests/test_tokens.py @@ -62,3 +62,10 @@ class TokenGeneratorTest(TestCase): # This will put a 14-digit base36 timestamp into the token, which is too large. with self.assertRaises(ValueError): p0._make_token_with_timestamp(user, 175455491841851871349) + + def test_check_token_with_nonexistent_token_and_user(self): + user = User.objects.create_user('tokentestuser', 'test2@example.com', 'testpw') + p0 = PasswordResetTokenGenerator() + tk1 = p0.make_token(user) + self.assertIs(p0.check_token(None, tk1), False) + self.assertIs(p0.check_token(user, None), False) diff --git a/tests/auth_tests/test_views.py b/tests/auth_tests/test_views.py index 209f9f698a..2d0d2ae96f 100644 --- a/tests/auth_tests/test_views.py +++ b/tests/auth_tests/test_views.py @@ -16,7 +16,8 @@ from django.contrib.auth.forms import ( ) from django.contrib.auth.models import User from django.contrib.auth.views import ( - LoginView, logout_then_login, redirect_to_login, + INTERNAL_RESET_SESSION_TOKEN, LoginView, logout_then_login, + redirect_to_login, ) from django.contrib.sessions.middleware import SessionMiddleware from django.contrib.sites.requests import RequestSite @@ -24,7 +25,7 @@ from django.core import mail from django.db import connection from django.http import HttpRequest, QueryDict from django.middleware.csrf import CsrfViewMiddleware, get_token -from django.test import TestCase, override_settings +from django.test import Client, TestCase, override_settings from django.test.utils import patch_logger from django.urls import NoReverseMatch, reverse, reverse_lazy from django.utils.deprecation import RemovedInDjango21Warning @@ -33,6 +34,7 @@ from django.utils.http import urlquote from django.utils.six.moves.urllib.parse import ParseResult, urlparse from django.utils.translation import LANGUAGE_SESSION_KEY +from .client import PasswordResetConfirmClient from .models import CustomUser, UUIDUser from .settings import AUTH_TEMPLATES @@ -116,6 +118,9 @@ class AuthViewNamedURLTests(AuthViewsTestCase): class PasswordResetTest(AuthViewsTestCase): + def setUp(self): + self.client = PasswordResetConfirmClient() + def test_email_not_found(self): """If the provided email is not registered, don't raise any error but also don't send any email.""" @@ -278,6 +283,8 @@ class PasswordResetTest(AuthViewsTestCase): # Check the password has been changed u = User.objects.get(email='staffmember@example.com') self.assertTrue(u.check_password("anewpassword")) + # The reset token is deleted from the session. + self.assertNotIn(INTERNAL_RESET_SESSION_TOKEN, self.client.session) # Check we can't use the link again response = self.client.get(path) @@ -338,6 +345,23 @@ class PasswordResetTest(AuthViewsTestCase): response = self.client.get('/reset/zzzzzzzzzzzzz/1-1/') self.assertContains(response, "Hello, .") + def test_confirm_link_redirects_to_set_password_page(self): + url, path = self._test_confirm_start() + # Don't use PasswordResetConfirmClient (self.client) here which + # automatically fetches the redirect page. + client = Client() + response = client.get(path) + token = response.resolver_match.kwargs['token'] + uuidb64 = response.resolver_match.kwargs['uidb64'] + self.assertRedirects(response, '/reset/%s/set-password/' % uuidb64) + self.assertEqual(client.session['_password_reset_token'], token) + + def test_invalid_link_if_going_directly_to_the_final_reset_password_url(self): + url, path = self._test_confirm_start() + _, uuidb64, _ = path.strip('/').split('/') + response = Client().get('/reset/%s/set-password/' % uuidb64) + self.assertContains(response, 'The password reset link was invalid') + @override_settings(AUTH_USER_MODEL='auth_tests.CustomUser') class CustomUserPasswordResetTest(AuthViewsTestCase): @@ -352,6 +376,9 @@ class CustomUserPasswordResetTest(AuthViewsTestCase): cls.u1.set_password('password') cls.u1.save() + def setUp(self): + self.client = PasswordResetConfirmClient() + def _test_confirm_start(self): # Start by creating the email response = self.client.post('/password_reset/', {'email': self.user_email})