From 0e5faf225c5cd1acf2ab653c74f5b161470403b9 Mon Sep 17 00:00:00 2001 From: Jacob Kaplan-Moss Date: Tue, 2 Sep 2008 21:10:00 +0000 Subject: [PATCH] Security fix. Announcement forthcoming. git-svn-id: http://code.djangoproject.com/svn/django/trunk@8877 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/contrib/admin/sites.py | 37 +------------ .../contrib/admin/templates/admin/login.html | 1 - django/contrib/admin/views/decorators.py | 41 +-------------- tests/regressiontests/admin_views/tests.py | 52 ++++++------------- 4 files changed, 19 insertions(+), 112 deletions(-) diff --git a/django/contrib/admin/sites.py b/django/contrib/admin/sites.py index dd35a03656..0be93a20c3 100644 --- a/django/contrib/admin/sites.py +++ b/django/contrib/admin/sites.py @@ -1,7 +1,5 @@ import base64 -import cPickle as pickle import re - from django import http, template from django.contrib.admin import ModelAdmin from django.contrib.auth import authenticate, login @@ -24,19 +22,6 @@ class AlreadyRegistered(Exception): class NotRegistered(Exception): 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): """ An AdminSite object encapsulates an instance of the Django admin application, ready @@ -239,7 +224,7 @@ class AdminSite(object): # If this isn't already the login page, display it. if not request.POST.has_key(LOGIN_FORM_KEY): 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: message = "" return self.display_login_form(request, message) @@ -275,15 +260,7 @@ class AdminSite(object): else: if user.is_active and user.is_staff: login(request, user) - if request.POST.has_key('post_data'): - 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()) + return http.HttpResponseRedirect(request.get_full_path()) else: return self.display_login_form(request, ERROR_MESSAGE) login = never_cache(login) @@ -345,19 +322,9 @@ class AdminSite(object): def display_login_form(self, request, error_message='', extra_context=None): 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 = { 'title': _('Log in'), 'app_path': request.get_full_path(), - 'post_data': post_data, 'error_message': error_message, 'root_path': self.root_path, } diff --git a/django/contrib/admin/templates/admin/login.html b/django/contrib/admin/templates/admin/login.html index 5dd953bc23..8c7603682b 100644 --- a/django/contrib/admin/templates/admin/login.html +++ b/django/contrib/admin/templates/admin/login.html @@ -21,7 +21,6 @@
- {#{% trans 'Have you forgotten your password?' %}#}
diff --git a/django/contrib/admin/views/decorators.py b/django/contrib/admin/views/decorators.py index f3c63ff70c..4566828b0a 100644 --- a/django/contrib/admin/views/decorators.py +++ b/django/contrib/admin/views/decorators.py @@ -1,5 +1,4 @@ import base64 -import cPickle as pickle try: from functools import wraps except ImportError: @@ -11,41 +10,18 @@ from django.contrib.auth.models import User from django.contrib.auth import authenticate, login from django.shortcuts import render_to_response 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.") LOGIN_FORM_KEY = 'this_is_the_login_form' def _display_login_form(request, error_message=''): 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', { 'title': _('Log in'), 'app_path': request.get_full_path(), - 'post_data': post_data, 'error_message': error_message }, 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): """ Decorator for views that checks that the user is logged in and is a staff @@ -54,10 +30,6 @@ def staff_member_required(view_func): def _checklogin(request, *args, **kwargs): if request.user.is_authenticated() and request.user.is_staff: # 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) 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'." @@ -65,7 +37,7 @@ def staff_member_required(view_func): # If this isn't already the login page, display it. if LOGIN_FORM_KEY not in 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: message = "" return _display_login_form(request, message) @@ -98,16 +70,7 @@ def staff_member_required(view_func): else: if user.is_active and user.is_staff: login(request, user) - # TODO: set last_login with an event. - 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()) + return http.HttpResponseRedirect(request.get_full_path()) else: return _display_login_form(request, ERROR_MESSAGE) diff --git a/tests/regressiontests/admin_views/tests.py b/tests/regressiontests/admin_views/tests.py index 56937d8462..5a10971570 100644 --- a/tests/regressiontests/admin_views/tests.py +++ b/tests/regressiontests/admin_views/tests.py @@ -4,7 +4,7 @@ from django.test import TestCase from django.contrib.auth.models import User, Permission from django.contrib.contenttypes.models import ContentType 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.utils.html import escape @@ -136,31 +136,31 @@ class AdminViewPermissionsTest(TestCase): Section._meta.get_delete_permission())) # login POST dicts - self.super_login = {'post_data': _encode_post_data({}), + self.super_login = { LOGIN_FORM_KEY: 1, 'username': 'super', 'password': 'secret'} - self.super_email_login = {'post_data': _encode_post_data({}), + self.super_email_login = { LOGIN_FORM_KEY: 1, 'username': 'super@example.com', 'password': 'secret'} - self.super_email_bad_login = {'post_data': _encode_post_data({}), + self.super_email_bad_login = { LOGIN_FORM_KEY: 1, 'username': 'super@example.com', 'password': 'notsecret'} - self.adduser_login = {'post_data': _encode_post_data({}), + self.adduser_login = { LOGIN_FORM_KEY: 1, 'username': 'adduser', 'password': 'secret'} - self.changeuser_login = {'post_data': _encode_post_data({}), + self.changeuser_login = { LOGIN_FORM_KEY: 1, 'username': 'changeuser', 'password': 'secret'} - self.deleteuser_login = {'post_data': _encode_post_data({}), + self.deleteuser_login = { LOGIN_FORM_KEY: 1, 'username': 'deleteuser', 'password': 'secret'} - self.joepublic_login = {'post_data': _encode_post_data({}), + self.joepublic_login = { LOGIN_FORM_KEY: 1, 'username': 'joepublic', 'password': 'secret'} @@ -271,17 +271,6 @@ class AdminViewPermissionsTest(TestCase): self.failUnlessEqual(Article.objects.all().count(), 3) 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 # to change user into the superuser without error login = self.client.login(username='joepublic', password='secret') @@ -489,31 +478,31 @@ class SecureViewTest(TestCase): def setUp(self): # login POST dicts - self.super_login = {'post_data': _encode_post_data({}), + self.super_login = { LOGIN_FORM_KEY: 1, 'username': 'super', 'password': 'secret'} - self.super_email_login = {'post_data': _encode_post_data({}), + self.super_email_login = { LOGIN_FORM_KEY: 1, 'username': 'super@example.com', 'password': 'secret'} - self.super_email_bad_login = {'post_data': _encode_post_data({}), + self.super_email_bad_login = { LOGIN_FORM_KEY: 1, 'username': 'super@example.com', 'password': 'notsecret'} - self.adduser_login = {'post_data': _encode_post_data({}), + self.adduser_login = { LOGIN_FORM_KEY: 1, 'username': 'adduser', 'password': 'secret'} - self.changeuser_login = {'post_data': _encode_post_data({}), + self.changeuser_login = { LOGIN_FORM_KEY: 1, 'username': 'changeuser', 'password': 'secret'} - self.deleteuser_login = {'post_data': _encode_post_data({}), + self.deleteuser_login = { LOGIN_FORM_KEY: 1, 'username': 'deleteuser', 'password': 'secret'} - self.joepublic_login = {'post_data': _encode_post_data({}), + self.joepublic_login = { LOGIN_FORM_KEY: 1, 'username': 'joepublic', 'password': 'secret'} @@ -597,17 +586,6 @@ class SecureViewTest(TestCase): # Login.context is a list of context dicts we just need to check the first one. 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 # to change user into the superuser without error login = self.client.login(username='joepublic', password='secret')