Merge pull request #1280 from erikr/improve-password-reset2

Fixed #20079 -- Improved security of password reset tokens

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("<strong>%s</strong>" % 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)

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:

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

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):

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

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]