From aeb1389442d0f9669edf6660b747fd10693b63a7 Mon Sep 17 00:00:00 2001 From: Erik Romijn Date: Tue, 18 Jun 2013 20:02:00 +0200 Subject: [PATCH] Fixed #20079 -- Improve security of password reset tokens --- django/contrib/auth/forms.py | 6 +++--- django/contrib/auth/hashers.py | 17 ++++++++++------- django/contrib/auth/models.py | 2 +- django/contrib/auth/tests/test_hashers.py | 11 ++++++++--- django/contrib/auth/tests/test_models.py | 2 +- tests/admin_views/tests.py | 8 ++++---- 6 files changed, 27 insertions(+), 19 deletions(-) diff --git a/django/contrib/auth/forms.py b/django/contrib/auth/forms.py index 028ec8eafc..a9ecba45c2 100644 --- a/django/contrib/auth/forms.py +++ b/django/contrib/auth/forms.py @@ -14,7 +14,7 @@ from django.utils.translation import ugettext, ugettext_lazy as _ from django.contrib.auth import authenticate, get_user_model from django.contrib.auth.models import User -from django.contrib.auth.hashers import UNUSABLE_PASSWORD, identify_hasher +from django.contrib.auth.hashers import UNUSABLE_PASSWORD_PREFIX, identify_hasher from django.contrib.auth.tokens import default_token_generator from django.contrib.sites.models import get_current_site @@ -29,7 +29,7 @@ class ReadOnlyPasswordHashWidget(forms.Widget): encoded = value final_attrs = self.build_attrs(attrs) - if not encoded or encoded == UNUSABLE_PASSWORD: + if not encoded or encoded.startswith(UNUSABLE_PASSWORD_PREFIX): summary = mark_safe("%s" % ugettext("No password set.")) else: try: @@ -231,7 +231,7 @@ class PasswordResetForm(forms.Form): for user in users: # Make sure that no email is sent to a user that actually has # a password marked as unusable - if user.password == UNUSABLE_PASSWORD: + if not user.has_usable_password(): continue if not domain_override: current_site = get_current_site(request) diff --git a/django/contrib/auth/hashers.py b/django/contrib/auth/hashers.py index d8a04e1473..87e4218a8f 100644 --- a/django/contrib/auth/hashers.py +++ b/django/contrib/auth/hashers.py @@ -17,7 +17,8 @@ from django.utils.module_loading import import_by_path from django.utils.translation import ugettext_noop as _ -UNUSABLE_PASSWORD = '!' # This will never be a valid encoded hash +UNUSABLE_PASSWORD_PREFIX = '!' # This will never be a valid encoded hash +UNUSABLE_PASSWORD_SUFFIX_LENGTH = 40 # number of random chars to add after UNUSABLE_PASSWORD_PREFIX HASHERS = None # lazily loaded from PASSWORD_HASHERS PREFERRED_HASHER = None # defaults to first item in PASSWORD_HASHERS @@ -30,7 +31,7 @@ def reset_hashers(**kwargs): def is_password_usable(encoded): - if encoded is None or encoded == UNUSABLE_PASSWORD: + if encoded is None or encoded.startswith(UNUSABLE_PASSWORD_PREFIX): return False try: hasher = identify_hasher(encoded) @@ -64,13 +65,15 @@ def make_password(password, salt=None, hasher='default'): """ Turn a plain-text password into a hash for database storage - Same as encode() but generates a new random salt. If - password is None then UNUSABLE_PASSWORD will be - returned which disallows logins. + Same as encode() but generates a new random salt. + If password is None then a concatenation of + UNUSABLE_PASSWORD_PREFIX and a random string will be returned + which disallows logins. Additional random string reduces chances + of gaining access to staff or superuser accounts. + See ticket #20079 for more info. """ if password is None: - return UNUSABLE_PASSWORD - + return UNUSABLE_PASSWORD_PREFIX + get_random_string(UNUSABLE_PASSWORD_SUFFIX_LENGTH) hasher = get_hasher(hasher) if not salt: diff --git a/django/contrib/auth/models.py b/django/contrib/auth/models.py index 798cc805a0..f6380b9f24 100644 --- a/django/contrib/auth/models.py +++ b/django/contrib/auth/models.py @@ -16,7 +16,7 @@ from django.utils import timezone from django.contrib import auth # UNUSABLE_PASSWORD is still imported here for backwards compatibility from django.contrib.auth.hashers import ( - check_password, make_password, is_password_usable, UNUSABLE_PASSWORD) + check_password, make_password, is_password_usable) from django.contrib.auth.signals import user_logged_in from django.contrib.contenttypes.models import ContentType from django.utils.encoding import python_2_unicode_compatible diff --git a/django/contrib/auth/tests/test_hashers.py b/django/contrib/auth/tests/test_hashers.py index 9df6bd8592..9b7811a335 100644 --- a/django/contrib/auth/tests/test_hashers.py +++ b/django/contrib/auth/tests/test_hashers.py @@ -3,8 +3,8 @@ from __future__ import unicode_literals from django.conf.global_settings import PASSWORD_HASHERS as default_hashers from django.contrib.auth.hashers import (is_password_usable, BasePasswordHasher, - check_password, make_password, PBKDF2PasswordHasher, load_hashers, - PBKDF2SHA1PasswordHasher, get_hasher, identify_hasher, UNUSABLE_PASSWORD) + check_password, make_password, PBKDF2PasswordHasher, load_hashers, PBKDF2SHA1PasswordHasher, + get_hasher, identify_hasher, UNUSABLE_PASSWORD_PREFIX, UNUSABLE_PASSWORD_SUFFIX_LENGTH) from django.utils import six from django.utils import unittest from django.utils.unittest import skipUnless @@ -173,13 +173,18 @@ class TestUtilsHashPass(unittest.TestCase): def test_unusable(self): encoded = make_password(None) + self.assertEqual(len(encoded), len(UNUSABLE_PASSWORD_PREFIX) + UNUSABLE_PASSWORD_SUFFIX_LENGTH) self.assertFalse(is_password_usable(encoded)) self.assertFalse(check_password(None, encoded)) - self.assertFalse(check_password(UNUSABLE_PASSWORD, encoded)) + self.assertFalse(check_password(encoded, encoded)) + self.assertFalse(check_password(UNUSABLE_PASSWORD_PREFIX, encoded)) self.assertFalse(check_password('', encoded)) self.assertFalse(check_password('lètmein', encoded)) self.assertFalse(check_password('lètmeinz', encoded)) self.assertRaises(ValueError, identify_hasher, encoded) + # Assert that the unusable passwords actually contain a random part. + # This might fail one day due to a hash collision. + self.assertNotEqual(encoded, make_password(None), "Random password collision?") def test_bad_algorithm(self): with self.assertRaises(ValueError): diff --git a/django/contrib/auth/tests/test_models.py b/django/contrib/auth/tests/test_models.py index 8ac0599e6b..cf412c96e6 100644 --- a/django/contrib/auth/tests/test_models.py +++ b/django/contrib/auth/tests/test_models.py @@ -87,7 +87,7 @@ class UserManagerTestCase(TestCase): user = User.objects.create_user('user', email_lowercase) self.assertEqual(user.email, email_lowercase) self.assertEqual(user.username, 'user') - self.assertEqual(user.password, '!') + self.assertFalse(user.has_usable_password()) def test_create_user_email_domain_normalize_rfc3696(self): # According to http://tools.ietf.org/html/rfc3696#section-3 diff --git a/tests/admin_views/tests.py b/tests/admin_views/tests.py index c0925e0ad9..9a7ec7d331 100644 --- a/tests/admin_views/tests.py +++ b/tests/admin_views/tests.py @@ -22,7 +22,7 @@ from django.contrib.admin.util import quote from django.contrib.admin.views.main import IS_POPUP_VAR from django.contrib.admin.tests import AdminSeleniumWebDriverTestCase from django.contrib.auth import REDIRECT_FIELD_NAME -from django.contrib.auth.models import Group, User, Permission, UNUSABLE_PASSWORD +from django.contrib.auth.models import Group, User, Permission from django.contrib.contenttypes.models import ContentType from django.core.urlresolvers import reverse from django.db import connection @@ -3632,7 +3632,7 @@ class UserAdminTest(TestCase): new_user = User.objects.order_by('-id')[0] self.assertRedirects(response, '/test_admin/admin/auth/user/%s/' % new_user.pk) self.assertEqual(User.objects.count(), user_count + 1) - self.assertNotEqual(new_user.password, UNUSABLE_PASSWORD) + self.assertTrue(new_user.has_usable_password()) def test_save_continue_editing_button(self): user_count = User.objects.count() @@ -3645,7 +3645,7 @@ class UserAdminTest(TestCase): new_user = User.objects.order_by('-id')[0] self.assertRedirects(response, '/test_admin/admin/auth/user/%s/' % new_user.pk) self.assertEqual(User.objects.count(), user_count + 1) - self.assertNotEqual(new_user.password, UNUSABLE_PASSWORD) + self.assertTrue(new_user.has_usable_password()) def test_password_mismatch(self): response = self.client.post('/test_admin/admin/auth/user/add/', { @@ -3691,7 +3691,7 @@ class UserAdminTest(TestCase): new_user = User.objects.order_by('-id')[0] self.assertRedirects(response, '/test_admin/admin/auth/user/add/') self.assertEqual(User.objects.count(), user_count + 1) - self.assertNotEqual(new_user.password, UNUSABLE_PASSWORD) + self.assertTrue(new_user.has_usable_password()) def test_user_permission_performance(self): u = User.objects.all()[0]