From 061d2ecc0e4b0a6d8c4a6fb18c2dcb482b41dd02 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Thu, 9 Dec 2010 21:53:15 +0000 Subject: [PATCH] BUGFIX Avoid potential referer leaking in Security->changepassword() form by storing Member->AutoLoginHash in session instead of 'h' GET parameter (from r114758) git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/branches/2.3@114763 467b73ca-7a2a-4603-9d3b-597d59a354a9 --- security/ChangePasswordForm.php | 15 +++++---- security/Security.php | 20 +++++++++--- tests/security/SecurityTest.php | 58 +++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 11 deletions(-) diff --git a/security/ChangePasswordForm.php b/security/ChangePasswordForm.php index 7c32d7e4e..468e651e8 100755 --- a/security/ChangePasswordForm.php +++ b/security/ChangePasswordForm.php @@ -5,7 +5,7 @@ * @subpackage security */ class ChangePasswordForm extends Form { - + /** * Constructor * @@ -22,7 +22,11 @@ class ChangePasswordForm extends Form { function __construct($controller, $name, $fields = null, $actions = null) { if(!$fields) { $fields = new FieldSet(); - if(Member::currentUser() && (!isset($_REQUEST['h']) || !Member::member_from_autologinhash($_REQUEST['h']))) { + + // Security/changepassword?h=XXX redirects to Security/changepassword + // without GET parameter to avoid potential HTTP referer leakage. + // In this case, a user is not logged in, and no 'old password' should be necessary. + if(Member::currentUser()) { $fields->push(new PasswordField("OldPassword",_t('Member.YOUROLDPASSWORD', "Your old password"))); } @@ -75,10 +79,9 @@ class ChangePasswordForm extends Form { if($data['NewPassword1'] == $data['NewPassword2']) { $isValid = $member->changePassword($data['NewPassword1']); if($isValid->valid()) { - $this->clearMessage(); - $this->sessionMessage( - _t('Member.PASSWORDCHANGED', "Your password has been changed, and a copy emailed to you."), - "good"); + $member->logIn(); + + // TODO Add confirmation message to login redirect Session::clear('AutoLoginHash'); $redirectURL = HTTP::setGetVar('BackURL', urlencode(Director::absoluteBaseURL()), Security::Link('login')); Director::redirect($redirectURL); diff --git a/security/Security.php b/security/Security.php index 960404e00..6791f14d6 100644 --- a/security/Security.php +++ b/security/Security.php @@ -471,7 +471,14 @@ class Security extends Controller { } /** - * Show the "change password" page + * Show the "change password" page. + * This page can either be called directly by logged-in users + * (in which case they need to provide their old password), + * or through a link emailed through {@link lostpassword()}. + * In this case no old password is required, authentication is ensured + * through the Member.AutoLoginHash property. + * + * @see ChangePasswordForm * * @return string Returns the "change password" page as HTML code. */ @@ -482,10 +489,15 @@ class Security extends Controller { $controller = new Page_Controller($tmpPage); $controller->init(); + // First load with hash: Redirect to same URL without hash to avoid referer leakage if(isset($_REQUEST['h']) && Member::member_from_autologinhash($_REQUEST['h'])) { - // The auto login hash is valid, store it for the change password form + // The auto login hash is valid, store it for the change password form. + // Temporary value, unset in ChangePasswordForm Session::set('AutoLoginHash', $_REQUEST['h']); - + + return $this->redirect($this->Link('changepassword')); + // Redirection target after "First load with hash" + } elseif(Session::get('AutoLoginHash')) { $customisedController = $controller->customise(array( 'Content' => '

' . @@ -493,7 +505,6 @@ class Security extends Controller { '

', 'Form' => $this->ChangePasswordForm(), )); - } elseif(Member::currentUser()) { // let a logged in user change his password $customisedController = $controller->customise(array( @@ -524,7 +535,6 @@ class Security extends Controller { } } - //Controller::$currentController = $controller; return $customisedController->renderWith(array('Security_changepassword', 'Security', $this->stat('template_main'))); } diff --git a/tests/security/SecurityTest.php b/tests/security/SecurityTest.php index 95d4b3d88..c288ec912 100644 --- a/tests/security/SecurityTest.php +++ b/tests/security/SecurityTest.php @@ -142,6 +142,51 @@ class SecurityTest extends FunctionalTest { $this->assertEquals($this->idFromFixture('Member', 'expiredpassword'), $this->session()->inst_get('loggedInAs')); } + + function testChangePasswordForLoggedInUsers() { + $goodResponse = $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword'); + + // Change the password + $this->get('Security/changepassword'); + $changedResponse = $this->doTestChangepasswordForm('1nitialPassword', 'changedPassword'); + $this->assertEquals(302, $changedResponse->getStatusCode()); + $this->assertEquals($this->idFromFixture('Member', 'test'), $this->session()->inst_get('loggedInAs')); + + // Check if we can login with the new password + $goodResponse = $this->doTestLoginForm('sam@silverstripe.com' , 'changedPassword'); + $this->assertEquals(302, $goodResponse->getStatusCode()); + $this->assertEquals($this->idFromFixture('Member', 'test'), $this->session()->inst_get('loggedInAs')); + } + + function testChangePasswordFromLostPassword() { + $admin = $this->objFromFixture('Member', 'test'); + + $this->assertNull($admin->AutoLoginHash, 'Hash is empty before lost password'); + + // Request new password by email + $response = $this->get('Security/lostpassword'); + $response = $this->submitForm('MemberLoginForm_LostPasswordForm', null, array('Email' => 'sam@silverstripe.com')); + + $this->assertEmailSent('sam@silverstripe.com'); + + // Load password link from email + $admin = DataObject::get_by_id('Member', $admin->ID); + $this->assertNotNull($admin->AutoLoginHash, 'Hash has been written after lost password'); + $response = $this->get('Security/changepassword/?h=' . $admin->AutoLoginHash); + $this->assertEquals(302, $response->getStatusCode()); + $this->assertEquals(Director::baseUrl() . 'Security/changepassword', $response->getHeader('Location')); + + // Follow redirection to form without hash in GET parameter + $response = $this->get('Security/changepassword'); + $changedResponse = $this->doTestChangepasswordForm('1nitialPassword', 'changedPassword'); + $this->assertEquals($this->idFromFixture('Member', 'test'), $this->session()->inst_get('loggedInAs')); + + // Check if we can login with the new password + $goodResponse = $this->doTestLoginForm('sam@silverstripe.com' , 'changedPassword'); + $this->assertEquals(302, $goodResponse->getStatusCode()); + $this->assertEquals($this->idFromFixture('Member', 'test'), $this->session()->inst_get('loggedInAs')); + } + function testRepeatedLoginAttemptsLockingPeopleOut() { Member::lock_out_after_incorrect_logins(5); @@ -283,6 +328,19 @@ class SecurityTest extends FunctionalTest { ); } + function doTestChangepasswordForm($oldPassword, $newPassword) { + return $this->submitForm( + "ChangePasswordForm_ChangePasswordForm", + null, + array( + 'OldPassword' => $oldPassword, + 'NewPassword1' => $newPassword, + 'NewPassword2' => $newPassword, + 'action_doChangePassword' => 1, + ) + ); + } + /** * Get the error message on the login form */