From 4b2c64c843093f2f08557ded108a5379b122097e Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Thu, 9 Dec 2010 21:18:49 +0000 Subject: [PATCH] BUGFIX Avoid potential referer leaking in Security->changepassword() form by storing Member->AutoLoginHash in session instead of 'h' GET parameter git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@114758 467b73ca-7a2a-4603-9d3b-597d59a354a9 --- security/ChangePasswordForm.php | 15 +++++++++------ security/Security.php | 20 +++++++++++++++----- tests/security/SecurityTest.php | 31 ++++++++++++++++++++++++++++++- 3 files changed, 54 insertions(+), 12 deletions(-) diff --git a/security/ChangePasswordForm.php b/security/ChangePasswordForm.php index 08af3f575..50e9bfd7d 100755 --- a/security/ChangePasswordForm.php +++ b/security/ChangePasswordForm.php @@ -5,7 +5,7 @@ * @subpackage security */ class ChangePasswordForm extends Form { - + /** * Constructor * @@ -28,7 +28,11 @@ class ChangePasswordForm extends Form { 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"))); } @@ -93,10 +97,9 @@ class ChangePasswordForm extends Form { else 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'); if (isset($_REQUEST['BackURL']) diff --git a/security/Security.php b/security/Security.php index c165c3e43..395912a33 100644 --- a/security/Security.php +++ b/security/Security.php @@ -519,7 +519,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. */ @@ -531,10 +538,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' => '

' . @@ -542,7 +554,6 @@ class Security extends Controller { '

', 'Form' => $this->ChangePasswordForm(), )); - } elseif(Member::currentUser()) { // let a logged in user change his password $customisedController = $controller->customise(array( @@ -573,7 +584,6 @@ class Security extends Controller { } } - //Controller::$currentController = $controller; return $customisedController->renderWith(array('Security_changepassword', 'Security', $this->stat('template_main'), 'ContentController')); } diff --git a/tests/security/SecurityTest.php b/tests/security/SecurityTest.php index e81ced568..1971fa064 100644 --- a/tests/security/SecurityTest.php +++ b/tests/security/SecurityTest.php @@ -165,7 +165,7 @@ class SecurityTest extends FunctionalTest { $this->assertEquals(Director::baseURL() . 'test/link', $changedResponse->getHeader('Location')); } - function testChangePassword() { + function testChangePasswordForLoggedInUsers() { $goodResponse = $this->doTestLoginForm('sam@silverstripe.com' , '1nitialPassword'); // Change the password @@ -181,6 +181,35 @@ class SecurityTest extends FunctionalTest { $this->assertEquals(Director::baseURL() . 'test/link', $goodResponse->getHeader('Location')); $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() { $local = i18n::get_locale();