Permalink
Browse files

Fixed #14445 - Use HMAC and constant-time comparison functions where …

…needed.

All adhoc MAC applications have been updated to use HMAC, using SHA1 to
generate unique keys for each application based on the SECRET_KEY, which is
common practice for this situation. In all cases, backwards compatibility
with existing hashes has been maintained, aiming to phase this out as per
the normal deprecation process. In this way, under most normal
circumstances the old hashes will have expired (e.g. by session expiration
etc.) before they become invalid.

In the case of the messages framework and the cookie backend, which was
already using HMAC, there is the possibility of a backwards incompatibility
if the SECRET_KEY is shorter than the default 50 bytes, but the low
likelihood and low impact meant compatibility code was not worth it.

All known instances where tokens/hashes were compared using simple string
equality, which could potentially open timing based attacks, have also been
fixed using a constant-time comparison function.

There are no known practical attacks against the existing implementations,
so these security improvements will not be backported.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@14218 bcc190cf-cafb-0310-a4f2-bffc1f526a37
  • Loading branch information...
1 parent 36f2f7e commit 45c7f427ce830dd1b2f636fb9c244fda9201cadb @spookylukey spookylukey committed Oct 14, 2010
@@ -50,3 +50,25 @@ def _today(self):
p2 = Mocked(date.today() + timedelta(settings.PASSWORD_RESET_TIMEOUT_DAYS + 1))
self.assertFalse(p2.check_token(user, tk1))
+
+ def test_django12_hash(self):
+ """
+ Ensure we can use the hashes generated by Django 1.2
+ """
+ # Hard code in the Django 1.2 algorithm (not the result, as it is time
+ # dependent)
+ def _make_token(user):
+ from django.utils.hashcompat import sha_constructor
+ from django.utils.http import int_to_base36
+
+ timestamp = (date.today() - date(2001,1,1)).days
+ ts_b36 = int_to_base36(timestamp)
+ hash = sha_constructor(settings.SECRET_KEY + unicode(user.id) +
+ user.password + user.last_login.strftime('%Y-%m-%d %H:%M:%S') +
+ unicode(timestamp)).hexdigest()[::2]
+ return "%s-%s" % (ts_b36, hash)
+
+ user = User.objects.create_user('tokentestuser', 'test2@example.com', 'testpw')
+ p0 = PasswordResetTokenGenerator()
+ tk1 = _make_token(user)
+ self.assertTrue(p0.check_token(user, tk1))
@@ -1,6 +1,9 @@
from datetime import date
+
from django.conf import settings
+from django.utils.hashcompat import sha_constructor
from django.utils.http import int_to_base36, base36_to_int
+from django.utils.crypto import constant_time_compare, salted_hmac
class PasswordResetTokenGenerator(object):
"""
@@ -30,8 +33,12 @@ def check_token(self, user, token):
return False
# Check that the timestamp/uid has not been tampered with
- if self._make_token_with_timestamp(user, ts) != token:
- return False
+ if not constant_time_compare(self._make_token_with_timestamp(user, ts), token):
+ # Fallback to Django 1.2 method for compatibility.
+ # PendingDeprecationWarning <- here to remind us to remove this in
+ # Django 1.5
+ if not constant_time_compare(self._make_token_with_timestamp_old(user, ts), token):
+ return False
# Check the timestamp is within limit
if (self._num_days(self._today()) - ts) > settings.PASSWORD_RESET_TIMEOUT_DAYS:
@@ -50,7 +57,16 @@ def _make_token_with_timestamp(self, user, timestamp):
# last_login will also change), we produce a hash that will be
# invalid as soon as it is used.
# We limit the hash to 20 chars to keep URL short
- from django.utils.hashcompat import sha_constructor
+ key_salt = "django.contrib.auth.tokens.PasswordResetTokenGenerator"
+ value = unicode(user.id) + \
+ user.password + user.last_login.strftime('%Y-%m-%d %H:%M:%S') + \
+ unicode(timestamp)
+ hash = salted_hmac(key_salt, value).hexdigest()[::2]
+ return "%s-%s" % (ts_b36, hash)
+
+ def _make_token_with_timestamp_old(self, user, timestamp):
+ # The Django 1.2 method
+ ts_b36 = int_to_base36(timestamp)
hash = sha_constructor(settings.SECRET_KEY + unicode(user.id) +
user.password + user.last_login.strftime('%Y-%m-%d %H:%M:%S') +
unicode(timestamp)).hexdigest()[::2]
@@ -6,6 +6,7 @@
from django.conf import settings
from django.contrib.contenttypes.models import ContentType
from models import Comment
+from django.utils.crypto import salted_hmac, constant_time_compare
from django.utils.encoding import force_unicode
from django.utils.hashcompat import sha_constructor
from django.utils.text import get_text_list
@@ -46,8 +47,13 @@ def clean_security_hash(self):
}
expected_hash = self.generate_security_hash(**security_hash_dict)
actual_hash = self.cleaned_data["security_hash"]
- if expected_hash != actual_hash:
- raise forms.ValidationError("Security hash check failed.")
+ if not constant_time_compare(expected_hash, actual_hash):
+ # Fallback to Django 1.2 method for compatibility
+ # PendingDeprecationWarning <- here to remind us to remove this
+ # fallback in Django 1.5
+ expected_hash_old = self._generate_security_hash_old(**security_hash_dict)
+ if not constant_time_compare(expected_hash_old, actual_hash):
+ raise forms.ValidationError("Security hash check failed.")
return actual_hash
def clean_timestamp(self):
@@ -82,7 +88,17 @@ def initial_security_hash(self, timestamp):
return self.generate_security_hash(**initial_security_dict)
def generate_security_hash(self, content_type, object_pk, timestamp):
+ """
+ Generate a HMAC security hash from the provided info.
+ """
+ info = (content_type, object_pk, timestamp)
+ key_salt = "django.contrib.forms.CommentSecurityForm"
+ value = "-".join(info)
+ return salted_hmac(key_salt, value).hexdigest()
+
+ def _generate_security_hash_old(self, content_type, object_pk, timestamp):
"""Generate a (SHA1) security hash from the provided info."""
+ # Django 1.2 compatibility
info = (content_type, object_pk, timestamp, settings.SECRET_KEY)
return sha_constructor("".join(info)).hexdigest()
@@ -9,6 +9,7 @@
from django.shortcuts import render_to_response
from django.template.context import RequestContext
from django.utils.hashcompat import md5_constructor
+from django.utils.crypto import constant_time_compare
from django.contrib.formtools.utils import security_hash
AUTO_ID = 'formtools_%s' # Each form here uses this as its auto_id parameter.
@@ -67,11 +68,33 @@ def preview_post(self, request):
else:
return render_to_response(self.form_template, context, context_instance=RequestContext(request))
+ def _check_security_hash(self, token, request, form):
+ expected = self.security_hash(request, form)
+ if constant_time_compare(token, expected):
+ return True
+ else:
+ # Fall back to Django 1.2 method, for compatibility with forms that
+ # are in the middle of being used when the upgrade occurs. However,
+ # we don't want to do this fallback if a subclass has provided their
+ # own security_hash method - because they might have implemented a
+ # more secure method, and this would punch a hole in that.
+
+ # PendingDeprecationWarning <- left here to remind us that this
+ # compatibility fallback should be removed in Django 1.5
+ FormPreview_expected = FormPreview.security_hash(self, request, form)
+ if expected == FormPreview_expected:
+ # They didn't override security_hash, do the fallback:
+ old_expected = security_hash(request, form)
+ return constant_time_compare(token, old_expected)
+ else:
+ return False
+
def post_post(self, request):
"Validates the POST data. If valid, calls done(). Else, redisplays form."
f = self.form(request.POST, auto_id=AUTO_ID)
if f.is_valid():
- if self.security_hash(request, f) != request.POST.get(self.unused_name('hash')):
+ if not self._check_security_hash(request.POST.get(self.unused_name('hash'), ''),
+ request, f):
return self.failed_hash(request) # Security hash failed.
return self.done(request, f.cleaned_data)
else:
@@ -1,10 +0,0 @@
-"""
-This is a URLconf to be loaded by tests.py. Add any URLs needed for tests only.
-"""
-
-from django.conf.urls.defaults import *
-from django.contrib.formtools.tests import *
-
-urlpatterns = patterns('',
- (r'^test1/', TestFormPreview(TestForm)),
- )
Oops, something went wrong.

0 comments on commit 45c7f42

Please sign in to comment.