Skip to content

Commit

Permalink
Security fix. Announcement forthcoming.
Browse files Browse the repository at this point in the history 
git-svn-id: http://code.djangoproject.com/svn/django/trunk@8877 bcc190cf-cafb-0310-a4f2-bffc1f526a37
  • Loading branch information
@jacobian
jacobian committed Sep 2, 2008
1 parent ecb5f4c commit 0e5faf2
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 112 deletions.
37 changes: 2 additions & 35 deletions django/contrib/admin/sites.py
View file
@@ -1,7 +1,5 @@
import base64 import base64
import cPickle as pickle
import re import re

from django import http, template from django import http, template
from django.contrib.admin import ModelAdmin from django.contrib.admin import ModelAdmin
from django.contrib.auth import authenticate, login from django.contrib.auth import authenticate, login
Expand All @@ -24,19 +22,6 @@ class AlreadyRegistered(Exception):
class NotRegistered(Exception): class NotRegistered(Exception):
pass pass


def _encode_post_data(post_data):
pickled = pickle.dumps(post_data)
pickled_md5 = md5_constructor(pickled + settings.SECRET_KEY).hexdigest()
return base64.encodestring(pickled + pickled_md5)

def _decode_post_data(encoded_data):
encoded_data = base64.decodestring(encoded_data)
pickled, tamper_check = encoded_data[:-32], encoded_data[-32:]
if md5_constructor(pickled + settings.SECRET_KEY).hexdigest() != tamper_check:
from django.core.exceptions import SuspiciousOperation
raise SuspiciousOperation, "User may have tampered with session cookie."
return pickle.loads(pickled)

class AdminSite(object): class AdminSite(object):
""" """
An AdminSite object encapsulates an instance of the Django admin application, ready An AdminSite object encapsulates an instance of the Django admin application, ready
Expand Down Expand Up @@ -239,7 +224,7 @@ def login(self, request):
# If this isn't already the login page, display it. # If this isn't already the login page, display it.
if not request.POST.has_key(LOGIN_FORM_KEY): if not request.POST.has_key(LOGIN_FORM_KEY):
if request.POST: if request.POST:
message = _("Please log in again, because your session has expired. Don't worry: Your submission has been saved.") message = _("Please log in again, because your session has expired.")
else: else:
message = "" message = ""
return self.display_login_form(request, message) return self.display_login_form(request, message)
Expand Down Expand Up @@ -275,15 +260,7 @@ def login(self, request):
else: else:
if user.is_active and user.is_staff: if user.is_active and user.is_staff:
login(request, user) login(request, user)
if request.POST.has_key('post_data'): return http.HttpResponseRedirect(request.get_full_path())
post_data = _decode_post_data(request.POST['post_data'])
if post_data and not post_data.has_key(LOGIN_FORM_KEY):
# overwrite request.POST with the saved post_data, and continue
request.POST = post_data
request.user = user
return self.root(request, request.path.split(self.root_path)[-1])
else:
return http.HttpResponseRedirect(request.get_full_path())
else: else:
return self.display_login_form(request, ERROR_MESSAGE) return self.display_login_form(request, ERROR_MESSAGE)
login = never_cache(login) login = never_cache(login)
Expand Down Expand Up @@ -345,19 +322,9 @@ def index(self, request, extra_context=None):


def display_login_form(self, request, error_message='', extra_context=None): def display_login_form(self, request, error_message='', extra_context=None):
request.session.set_test_cookie() request.session.set_test_cookie()
if request.POST and request.POST.has_key('post_data'):
# User has failed login BUT has previously saved post data.
post_data = request.POST['post_data']
elif request.POST:
# User's session must have expired; save their post data.
post_data = _encode_post_data(request.POST)
else:
post_data = _encode_post_data({})

context = { context = {
'title': _('Log in'), 'title': _('Log in'),
'app_path': request.get_full_path(), 'app_path': request.get_full_path(),
'post_data': post_data,
'error_message': error_message, 'error_message': error_message,
'root_path': self.root_path, 'root_path': self.root_path,
} }
Expand Down
1 change: 0 additions & 1 deletion django/contrib/admin/templates/admin/login.html
View file
Expand Up @@ -21,7 +21,6 @@
<div class="form-row"> <div class="form-row">
<label for="id_password">{% trans 'Password:' %}</label> <input type="password" name="password" id="id_password" /> <label for="id_password">{% trans 'Password:' %}</label> <input type="password" name="password" id="id_password" />
<input type="hidden" name="this_is_the_login_form" value="1" /> <input type="hidden" name="this_is_the_login_form" value="1" />
<input type="hidden" name="post_data" value="{{ post_data }}" /> {#<span class="help">{% trans 'Have you <a href="/password_reset/">forgotten your password</a>?' %}</span>#}
</div> </div>
<div class="submit-row"> <div class="submit-row">
<label>&nbsp;</label><input type="submit" value="{% trans 'Log in' %}" /> <label>&nbsp;</label><input type="submit" value="{% trans 'Log in' %}" />
Expand Down
41 changes: 2 additions & 39 deletions django/contrib/admin/views/decorators.py
View file
@@ -1,5 +1,4 @@
import base64 import base64
import cPickle as pickle
try: try:
from functools import wraps from functools import wraps
except ImportError: except ImportError:
Expand All @@ -11,41 +10,18 @@
from django.contrib.auth import authenticate, login from django.contrib.auth import authenticate, login
from django.shortcuts import render_to_response from django.shortcuts import render_to_response
from django.utils.translation import ugettext_lazy, ugettext as _ from django.utils.translation import ugettext_lazy, ugettext as _
from django.utils.hashcompat import md5_constructor


ERROR_MESSAGE = ugettext_lazy("Please enter a correct username and password. Note that both fields are case-sensitive.") ERROR_MESSAGE = ugettext_lazy("Please enter a correct username and password. Note that both fields are case-sensitive.")
LOGIN_FORM_KEY = 'this_is_the_login_form' LOGIN_FORM_KEY = 'this_is_the_login_form'


def _display_login_form(request, error_message=''): def _display_login_form(request, error_message=''):
request.session.set_test_cookie() request.session.set_test_cookie()
if request.POST and 'post_data' in request.POST:
# User has failed login BUT has previously saved post data.
post_data = request.POST['post_data']
elif request.POST:
# User's session must have expired; save their post data.
post_data = _encode_post_data(request.POST)
else:
post_data = _encode_post_data({})
return render_to_response('admin/login.html', { return render_to_response('admin/login.html', {
'title': _('Log in'), 'title': _('Log in'),
'app_path': request.get_full_path(), 'app_path': request.get_full_path(),
'post_data': post_data,
'error_message': error_message 'error_message': error_message
}, context_instance=template.RequestContext(request)) }, context_instance=template.RequestContext(request))


def _encode_post_data(post_data):
pickled = pickle.dumps(post_data)
pickled_md5 = md5_constructor(pickled + settings.SECRET_KEY).hexdigest()
return base64.encodestring(pickled + pickled_md5)

def _decode_post_data(encoded_data):
encoded_data = base64.decodestring(encoded_data)
pickled, tamper_check = encoded_data[:-32], encoded_data[-32:]
if md5_constructor(pickled + settings.SECRET_KEY).hexdigest() != tamper_check:
from django.core.exceptions import SuspiciousOperation
raise SuspiciousOperation, "User may have tampered with session cookie."
return pickle.loads(pickled)

def staff_member_required(view_func): def staff_member_required(view_func):
""" """
Decorator for views that checks that the user is logged in and is a staff Decorator for views that checks that the user is logged in and is a staff
Expand All @@ -54,18 +30,14 @@ def staff_member_required(view_func):
def _checklogin(request, *args, **kwargs): def _checklogin(request, *args, **kwargs):
if request.user.is_authenticated() and request.user.is_staff: if request.user.is_authenticated() and request.user.is_staff:
# The user is valid. Continue to the admin page. # The user is valid. Continue to the admin page.
if 'post_data' in request.POST:
# User must have re-authenticated through a different window
# or tab.
request.POST = _decode_post_data(request.POST['post_data'])
return view_func(request, *args, **kwargs) return view_func(request, *args, **kwargs)


assert hasattr(request, 'session'), "The Django admin requires session middleware to be installed. Edit your MIDDLEWARE_CLASSES setting to insert 'django.contrib.sessions.middleware.SessionMiddleware'." assert hasattr(request, 'session'), "The Django admin requires session middleware to be installed. Edit your MIDDLEWARE_CLASSES setting to insert 'django.contrib.sessions.middleware.SessionMiddleware'."


# If this isn't already the login page, display it. # If this isn't already the login page, display it.
if LOGIN_FORM_KEY not in request.POST: if LOGIN_FORM_KEY not in request.POST:
if request.POST: if request.POST:
message = _("Please log in again, because your session has expired. Don't worry: Your submission has been saved.") message = _("Please log in again, because your session has expired.")
else: else:
message = "" message = ""
return _display_login_form(request, message) return _display_login_form(request, message)
Expand Down Expand Up @@ -98,16 +70,7 @@ def _checklogin(request, *args, **kwargs):
else: else:
if user.is_active and user.is_staff: if user.is_active and user.is_staff:
login(request, user) login(request, user)
# TODO: set last_login with an event. return http.HttpResponseRedirect(request.get_full_path())
if 'post_data' in request.POST:
post_data = _decode_post_data(request.POST['post_data'])
if post_data and LOGIN_FORM_KEY not in post_data:
# overwrite request.POST with the saved post_data, and continue
request.POST = post_data
request.user = user
return view_func(request, *args, **kwargs)
else:
return http.HttpResponseRedirect(request.get_full_path())
else: else:
return _display_login_form(request, ERROR_MESSAGE) return _display_login_form(request, ERROR_MESSAGE)


Expand Down
52 changes: 15 additions & 37 deletions tests/regressiontests/admin_views/tests.py
View file
Expand Up @@ -4,7 +4,7 @@
from django.contrib.auth.models import User, Permission from django.contrib.auth.models import User, Permission
from django.contrib.contenttypes.models import ContentType from django.contrib.contenttypes.models import ContentType
from django.contrib.admin.models import LogEntry from django.contrib.admin.models import LogEntry
from django.contrib.admin.sites import LOGIN_FORM_KEY, _encode_post_data from django.contrib.admin.sites import LOGIN_FORM_KEY
from django.contrib.admin.util import quote from django.contrib.admin.util import quote
from django.utils.html import escape from django.utils.html import escape


Expand Down Expand Up @@ -136,31 +136,31 @@ def setUp(self):
Section._meta.get_delete_permission())) Section._meta.get_delete_permission()))


# login POST dicts # login POST dicts
self.super_login = {'post_data': _encode_post_data({}), self.super_login = {
LOGIN_FORM_KEY: 1, LOGIN_FORM_KEY: 1,
'username': 'super', 'username': 'super',
'password': 'secret'} 'password': 'secret'}
self.super_email_login = {'post_data': _encode_post_data({}), self.super_email_login = {
LOGIN_FORM_KEY: 1, LOGIN_FORM_KEY: 1,
'username': 'super@example.com', 'username': 'super@example.com',
'password': 'secret'} 'password': 'secret'}
self.super_email_bad_login = {'post_data': _encode_post_data({}), self.super_email_bad_login = {
LOGIN_FORM_KEY: 1, LOGIN_FORM_KEY: 1,
'username': 'super@example.com', 'username': 'super@example.com',
'password': 'notsecret'} 'password': 'notsecret'}
self.adduser_login = {'post_data': _encode_post_data({}), self.adduser_login = {
LOGIN_FORM_KEY: 1, LOGIN_FORM_KEY: 1,
'username': 'adduser', 'username': 'adduser',
'password': 'secret'} 'password': 'secret'}
self.changeuser_login = {'post_data': _encode_post_data({}), self.changeuser_login = {
LOGIN_FORM_KEY: 1, LOGIN_FORM_KEY: 1,
'username': 'changeuser', 'username': 'changeuser',
'password': 'secret'} 'password': 'secret'}
self.deleteuser_login = {'post_data': _encode_post_data({}), self.deleteuser_login = {
LOGIN_FORM_KEY: 1, LOGIN_FORM_KEY: 1,
'username': 'deleteuser', 'username': 'deleteuser',
'password': 'secret'} 'password': 'secret'}
self.joepublic_login = {'post_data': _encode_post_data({}), self.joepublic_login = {
LOGIN_FORM_KEY: 1, LOGIN_FORM_KEY: 1,
'username': 'joepublic', 'username': 'joepublic',
'password': 'secret'} 'password': 'secret'}
Expand Down Expand Up @@ -271,17 +271,6 @@ def testAddView(self):
self.failUnlessEqual(Article.objects.all().count(), 3) self.failUnlessEqual(Article.objects.all().count(), 3)
self.client.get('/test_admin/admin/logout/') self.client.get('/test_admin/admin/logout/')


# Check and make sure that if user expires, data still persists
post = self.client.post('/test_admin/admin/admin_views/article/add/', add_dict)
self.assertContains(post, 'Please log in again, because your session has expired.')
self.super_login['post_data'] = _encode_post_data(add_dict)
post = self.client.post('/test_admin/admin/admin_views/article/add/', self.super_login)
# make sure the view removes test cookie
self.failUnlessEqual(self.client.session.test_cookie_worked(), False)
self.assertRedirects(post, '/test_admin/admin/admin_views/article/')
self.failUnlessEqual(Article.objects.all().count(), 4)
self.client.get('/test_admin/admin/logout/')

# 8509 - if a normal user is already logged in, it is possible # 8509 - if a normal user is already logged in, it is possible
# to change user into the superuser without error # to change user into the superuser without error
login = self.client.login(username='joepublic', password='secret') login = self.client.login(username='joepublic', password='secret')
Expand Down Expand Up @@ -489,31 +478,31 @@ class SecureViewTest(TestCase):


def setUp(self): def setUp(self):
# login POST dicts # login POST dicts
self.super_login = {'post_data': _encode_post_data({}), self.super_login = {
LOGIN_FORM_KEY: 1, LOGIN_FORM_KEY: 1,
'username': 'super', 'username': 'super',
'password': 'secret'} 'password': 'secret'}
self.super_email_login = {'post_data': _encode_post_data({}), self.super_email_login = {
LOGIN_FORM_KEY: 1, LOGIN_FORM_KEY: 1,
'username': 'super@example.com', 'username': 'super@example.com',
'password': 'secret'} 'password': 'secret'}
self.super_email_bad_login = {'post_data': _encode_post_data({}), self.super_email_bad_login = {
LOGIN_FORM_KEY: 1, LOGIN_FORM_KEY: 1,
'username': 'super@example.com', 'username': 'super@example.com',
'password': 'notsecret'} 'password': 'notsecret'}
self.adduser_login = {'post_data': _encode_post_data({}), self.adduser_login = {
LOGIN_FORM_KEY: 1, LOGIN_FORM_KEY: 1,
'username': 'adduser', 'username': 'adduser',
'password': 'secret'} 'password': 'secret'}
self.changeuser_login = {'post_data': _encode_post_data({}), self.changeuser_login = {
LOGIN_FORM_KEY: 1, LOGIN_FORM_KEY: 1,
'username': 'changeuser', 'username': 'changeuser',
'password': 'secret'} 'password': 'secret'}
self.deleteuser_login = {'post_data': _encode_post_data({}), self.deleteuser_login = {
LOGIN_FORM_KEY: 1, LOGIN_FORM_KEY: 1,
'username': 'deleteuser', 'username': 'deleteuser',
'password': 'secret'} 'password': 'secret'}
self.joepublic_login = {'post_data': _encode_post_data({}), self.joepublic_login = {
LOGIN_FORM_KEY: 1, LOGIN_FORM_KEY: 1,
'username': 'joepublic', 'username': 'joepublic',
'password': 'secret'} 'password': 'secret'}
Expand Down Expand Up @@ -597,17 +586,6 @@ def test_staff_member_required_decorator_works_as_per_admin_login(self):
# Login.context is a list of context dicts we just need to check the first one. # Login.context is a list of context dicts we just need to check the first one.
self.assert_(login.context[0].get('error_message')) self.assert_(login.context[0].get('error_message'))


# Check and make sure that if user expires, data still persists
data = {'foo': 'bar'}
post = self.client.post('/test_admin/admin/secure-view/', data)
self.assertContains(post, 'Please log in again, because your session has expired.')
self.super_login['post_data'] = _encode_post_data(data)
post = self.client.post('/test_admin/admin/secure-view/', self.super_login)
# make sure the view removes test cookie
self.failUnlessEqual(self.client.session.test_cookie_worked(), False)
self.assertContains(post, "{'foo': 'bar'}")
self.client.get('/test_admin/admin/logout/')

# 8509 - if a normal user is already logged in, it is possible # 8509 - if a normal user is already logged in, it is possible
# to change user into the superuser without error # to change user into the superuser without error
login = self.client.login(username='joepublic', password='secret') login = self.client.login(username='joepublic', password='secret')
Expand Down

0 comments on commit 0e5faf2

Please sign in to comment.