Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

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

Fixed #20079 -- Improved security of password reset tokens
  • Loading branch information...
commit a01b1ee6881cc4ce6c8bee771bb5b463bc16dd77 2 parents 2c4fe76 + aeb1389
@aaugustin aaugustin authored
View
6 django/contrib/auth/forms.py
@@ -14,7 +14,7 @@
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 @@ def render(self, name, value, attrs):
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 @@ def save(self, domain_override=None,
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)
View
17 django/contrib/auth/hashers.py
@@ -17,7 +17,8 @@
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:
View
2  django/contrib/auth/models.py
@@ -16,7 +16,7 @@
from django.contrib import auth
# UNUSABLE_PASSWORD is still imported here for backwards compatibility

This comment isn't correct any more

@aaugustin Owner

Fixed in c8756e1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
View
11 django/contrib/auth/tests/test_hashers.py
@@ -3,8 +3,8 @@
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 @@ def test_bcrypt(self):
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):
View
2  django/contrib/auth/tests/test_models.py
@@ -87,7 +87,7 @@ def test_create_user(self):
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
View
8 tests/admin_views/tests.py
@@ -22,7 +22,7 @@
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 @@ def test_save_button(self):
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 @@ def test_save_continue_editing_button(self):
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 @@ def test_save_add_another_button(self):
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]
@gavinwahl

This comment isn't correct any more

Please sign in to comment.
Something went wrong with that request. Please try again.