diff --git a/security/Member.php b/security/Member.php
index 44ef14902..30794ebf5 100644
--- a/security/Member.php
+++ b/security/Member.php
@@ -12,11 +12,11 @@ class Member extends DataObject implements TemplateGlobalProvider {
'Surname' => 'Varchar',
'Email' => 'Varchar(256)', // See RFC 5321, Section 4.5.3.1.3.
'Password' => 'Varchar(160)',
- 'RememberLoginToken' => 'Varchar(50)',
+ 'RememberLoginToken' => 'Varchar(160)', // Note: this currently holds a hash, not a token.
'NumVisit' => 'Int',
'LastVisited' => 'SS_Datetime',
'Bounced' => 'Boolean', // Note: This does not seem to be used anywhere.
- 'AutoLoginHash' => 'Varchar(50)',
+ 'AutoLoginHash' => 'Varchar(160)',
'AutoLoginExpired' => 'SS_Datetime',
// This is an arbitrary code pointing to a PasswordEncryptor instance,
// not an actual encryption algorithm.
@@ -322,9 +322,11 @@ class Member extends DataObject implements TemplateGlobalProvider {
$this->NumVisit++;
if($remember) {
+ // Store the hash and give the client the cookie with the token.
$generator = new RandomGenerator();
- $token = $generator->generateHash('sha1');
- $this->RememberLoginToken = $token;
+ $token = $generator->randomToken('sha1');
+ $hash = $this->encryptWithUserSettings($token);
+ $this->RememberLoginToken = $hash;
Cookie::set('alc_enc', $this->ID . ':' . $token, 90, null, null, null, true);
} else {
$this->RememberLoginToken = null;
@@ -382,7 +384,8 @@ class Member extends DataObject implements TemplateGlobalProvider {
$member = DataObject::get_one("Member", "\"Member\".\"ID\" = '$SQL_uid'");
// check if autologin token matches
- if($member && (!$member->RememberLoginToken || $member->RememberLoginToken != $token)) {
+ $hash = $member->encryptWithUserSettings($token);
+ if($member && (!$member->RememberLoginToken || $member->RememberLoginToken != $hash)) {
$member = null;
}
@@ -393,8 +396,10 @@ class Member extends DataObject implements TemplateGlobalProvider {
if(self::$login_marker_cookie) Cookie::set(self::$login_marker_cookie, 1, 0, null, null, false, true);
$generator = new RandomGenerator();
- $member->RememberLoginToken = $generator->generateHash('sha1');
- Cookie::set('alc_enc', $member->ID . ':' . $member->RememberLoginToken, 90, null, null, false, true);
+ $token = $generator->randomToken('sha1');
+ $hash = $member->encryptWithUserSettings($token);
+ $member->RememberLoginToken = $hash;
+ Cookie::set('alc_enc', $member->ID . ':' . $token, 90, null, null, false, true);
$member->NumVisit++;
$member->write();
@@ -425,27 +430,82 @@ class Member extends DataObject implements TemplateGlobalProvider {
$this->extend('memberLoggedOut');
}
+ /**
+ * Utility for generating secure password hashes for this member.
+ */
+ public function encryptWithUserSettings($string) {
+ if (!$string) return null;
+
+ // If the algorithm or salt is not available, it means we are operating
+ // on legacy account with unhashed password. Do not hash the string.
+ if (!$this->PasswordEncryption) {
+ return $string;
+ }
+
+ // We assume we have PasswordEncryption and Salt available here.
+ $e = PasswordEncryptor::create_for_algorithm($this->PasswordEncryption);
+ return $e->encrypt($string, $this->Salt);
+
+ }
/**
- * Generate an auto login hash
- *
- * This creates an auto login hash that can be used to reset the password.
+ * Generate an auto login token which can be used to reset the password,
+ * at the same time hashing it and storing in the database.
*
* @param int $lifetime The lifetime of the auto login hash in days (by default 2 days)
*
+ * @returns string Token that should be passed to the client (but NOT persisted).
+ *
* @todo Make it possible to handle database errors such as a "duplicate key" error
*/
- public function generateAutologinHash($lifetime = 2) {
-
+ public function generateAutologinTokenAndStoreHash($lifetime = 2) {
do {
$generator = new RandomGenerator();
- $hash = $generator->generateHash('sha1');
+ $token = $generator->randomToken();
+ $hash = $this->encryptWithUserSettings($token);
} while(DataObject::get_one('Member', "\"AutoLoginHash\" = '$hash'"));
$this->AutoLoginHash = $hash;
$this->AutoLoginExpired = date('Y-m-d', time() + (86400 * $lifetime));
$this->write();
+
+ return $token;
+ }
+
+ /**
+ * @deprecated 3.0
+ */
+ public function generateAutologinHash($lifetime = 2) {
+ Deprecation::notice('3.0',
+ 'Member::generateAutologinHash is deprecated - tokens are no longer saved directly into the database '.
+ 'in plaintext. Use the return value of the Member::generateAutologinTokenAndHash to get the token '.
+ 'instead.',
+ Deprecation::SCOPE_METHOD);
+
+ user_error(
+ 'Member::generateAutologinHash is deprecated - tokens are no longer saved directly into the database '.
+ 'in plaintext. Use the return value of the Member::generateAutologinTokenAndHash to get the token '.
+ 'instead.',
+ E_USER_ERROR);
+ }
+
+ /**
+ * Check the token against the member.
+ *
+ * @param string $autologinToken
+ *
+ * @returns bool Is token valid?
+ */
+ public function validateAutoLoginToken($autologinToken) {
+ $hash = $this->encryptWithUserSettings($autologinToken);
+
+ $member = DataObject::get_one(
+ 'Member',
+ "\"AutoLoginHash\"='" . $hash . "' AND \"AutoLoginExpired\" > " . DB::getConn()->now()
+ );
+
+ return (bool)$member;
}
/**
@@ -467,7 +527,6 @@ class Member extends DataObject implements TemplateGlobalProvider {
return $member;
}
-
/**
* Send signup, change password or forgot password informations to an user
*
diff --git a/security/MemberLoginForm.php b/security/MemberLoginForm.php
index a250c28bf..2e04763f4 100644
--- a/security/MemberLoginForm.php
+++ b/security/MemberLoginForm.php
@@ -258,12 +258,12 @@ JS
$member = DataObject::get_one('Member', "\"Email\" = '{$SQL_email}'");
if($member) {
- $member->generateAutologinHash();
+ $token = $member->generateAutologinTokenAndStoreHash();
$e = Member_ForgotPasswordEmail::create();
$e->populateTemplate($member);
$e->populateTemplate(array(
- 'PasswordResetLink' => Security::getPasswordResetLink($member->AutoLoginHash)
+ 'PasswordResetLink' => Security::getPasswordResetLink($member, $token)
));
$e->setTo($member->Email);
$e->send();
diff --git a/security/PasswordEncryptor.php b/security/PasswordEncryptor.php
index d2258d307..6c84f847d 100644
--- a/security/PasswordEncryptor.php
+++ b/security/PasswordEncryptor.php
@@ -99,7 +99,7 @@ abstract class PasswordEncryptor {
*/
public function salt($password, $member = null) {
$generator = new RandomGenerator();
- return substr($generator->generateHash('sha1'), 0, 50);
+ return substr($generator->randomToken('sha1'), 0, 50);
}
/**
@@ -281,7 +281,7 @@ class PasswordEncryptor_Blowfish extends PasswordEncryptor {
*/
public function salt($password, $member = null) {
$generator = new RandomGenerator();
- return sprintf('%02d', self::$cost) . '$' . substr($generator->generateHash('sha1'), 0, 22);
+ return sprintf('%02d', self::$cost) . '$' . substr($generator->randomToken('sha1'), 0, 22);
}
public function check($hash, $password, $salt = null, $member = null) {
diff --git a/security/RandomGenerator.php b/security/RandomGenerator.php
index 3751e38c2..3f33288be 100644
--- a/security/RandomGenerator.php
+++ b/security/RandomGenerator.php
@@ -58,15 +58,31 @@ class RandomGenerator {
// Fallback to good old mt_rand()
return uniqid(mt_rand(), true);
}
-
+
/**
- * Generates a hash suitable for manual session identifiers, CSRF tokens, etc.
+ * Generates a random token that can be used for session IDs, CSRF tokens etc., based on
+ * hash algorithms.
+ *
+ * If you are using it as a password equivalent (e.g. autologin token) do NOT store it
+ * in the database as a plain text but encrypt it with Member::encryptWithUserSettings.
*
* @param String $algorithm Any identifier listed in hash_algos() (Default: whirlpool)
- * If possible, choose a slow algorithm which complicates brute force attacks.
+ *
* @return String Returned length will depend on the used $algorithm
*/
- public function generateHash($algorithm = 'whirlpool') {
+ public function randomToken($algorithm = 'whirlpool') {
return hash($algorithm, $this->generateEntropy());
- }
-}
\ No newline at end of file
+ }
+
+ /**
+ * @deprecated 3.1
+ */
+ public function generateHash($algorithm = 'whirlpool') {
+ Deprecation::notice('3.1',
+ 'RandomGenerator::generateHash is deprecated because of a confusing name that hints the output is secure, '.
+ 'while in fact it is just a random string. Use RandomGenerator::randomToken instead.',
+ Deprecation::SCOPE_METHOD);
+
+ return $this->randomToken($algorithm);
+ }
+}
diff --git a/security/Security.php b/security/Security.php
index 1bc6ac837..908cb13a0 100644
--- a/security/Security.php
+++ b/security/Security.php
@@ -532,15 +532,20 @@ class Security extends Controller {
/**
- * Create a link to the password reset form
+ * Create a link to the password reset form.
*
- * @param string $autoLoginHash The auto login hash
+ * GET parameters used:
+ * - m: member ID
+ * - t: plaintext token
+ *
+ * @param Member $member Member object associated with this link.
+ * @param string $autoLoginHash The auto login token.
*/
- public static function getPasswordResetLink($autoLoginHash) {
- $autoLoginHash = urldecode($autoLoginHash);
+ public static function getPasswordResetLink($member, $autologinToken) {
+ $autologinToken = urldecode($autologinToken);
$selfControllerClass = __CLASS__;
$selfController = new $selfControllerClass();
- return $selfController->Link('changepassword') . "?h=$autoLoginHash";
+ return $selfController->Link('changepassword') . "?m={$member->ID}&t=$autologinToken";
}
/**
@@ -567,15 +572,22 @@ class Security extends Controller {
$controller = $this;
}
- // 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.
- // Temporary value, unset in ChangePasswordForm
- Session::set('AutoLoginHash', $_REQUEST['h']);
+ // Extract the member from the URL.
+ $member = null;
+ if (isset($_REQUEST['m'])) {
+ $member = Member::get()->filter('ID', (int)$_REQUEST['m'])->First();
+ }
+
+ // Check whether we are merely changin password, or resetting.
+ if(isset($_REQUEST['t']) && $member && $member->validateAutoLoginToken($_REQUEST['t'])) {
+ // On first valid password reset request redirect to the same URL without hash to avoid referrer leakage.
+
+ // Store the hash for the change password form. Will be unset after reload within the ChangePasswordForm.
+ Session::set('AutoLoginHash', $member->encryptWithUserSettings($_REQUEST['t']));
return $this->redirect($this->Link('changepassword'));
- // Redirection target after "First load with hash"
} elseif(Session::get('AutoLoginHash')) {
+ // Subsequent request after the "first load with hash" (see previous if clause).
$customisedController = $controller->customise(array(
'Content' =>
'' .
@@ -584,16 +596,16 @@ class Security extends Controller {
'Form' => $this->ChangePasswordForm(),
));
} elseif(Member::currentUser()) {
- // let a logged in user change his password
+ // Logged in user requested a password change form.
$customisedController = $controller->customise(array(
'Content' => '
'
. _t('Security.CHANGEPASSWORDBELOW', 'You can change your password below.') . '
',
'Form' => $this->ChangePasswordForm()));
} else {
- // show an error message if the auto login hash is invalid and the
+ // show an error message if the auto login token is invalid and the
// user is not logged in
- if(isset($_REQUEST['h'])) {
+ if(!isset($_REQUEST['t']) || !$member) {
$customisedController = $controller->customise(
array('Content' =>
_t(
diff --git a/security/SecurityToken.php b/security/SecurityToken.php
index d4a7bb8aa..444f87718 100644
--- a/security/SecurityToken.php
+++ b/security/SecurityToken.php
@@ -227,7 +227,7 @@ class SecurityToken extends Object implements TemplateGlobalProvider {
*/
protected function generate() {
$generator = new RandomGenerator();
- return $generator->generateHash('sha1');
+ return $generator->randomToken('sha1');
}
public static function get_template_global_variables() {
@@ -299,4 +299,4 @@ class NullSecurityToken extends SecurityToken {
public function generate() {
return null;
}
-}
\ No newline at end of file
+}
diff --git a/tests/security/MemberTest.php b/tests/security/MemberTest.php
index a08f4f552..68390b3db 100644
--- a/tests/security/MemberTest.php
+++ b/tests/security/MemberTest.php
@@ -624,6 +624,30 @@ class MemberTest extends FunctionalTest {
return $extensions;
}
+ public function testGenerateAutologinTokenAndStoreHash() {
+ $enc = new PasswordEncryptor_Blowfish();
+
+ $m = new Member();
+ $m->PasswordEncryption = 'blowfish';
+ $m->Salt = $enc->salt('123');
+
+ $token = $m->generateAutologinTokenAndStoreHash();
+
+ $this->assertEquals($m->encryptWithUserSettings($token), $m->AutoLoginHash, 'Stores the token as ahash.');
+ }
+
+ public function testValidateAutoLoginToken() {
+ $enc = new PasswordEncryptor_Blowfish();
+
+ $m = new Member();
+ $m->PasswordEncryption = 'blowfish';
+ $m->Salt = $enc->salt('123');
+
+ $token = $m->generateAutologinTokenAndStoreHash();
+
+ $this->assertTrue($m->validateAutoLoginToken($token), 'Tests the token validity against member correctly.');
+ }
+
}
class MemberTest_ViewingAllowedExtension extends DataExtension implements TestOnly {
diff --git a/tests/security/RandomGeneratorTest.php b/tests/security/RandomGeneratorTest.php
index 2baf6cef0..f85f3ae0d 100644
--- a/tests/security/RandomGeneratorTest.php
+++ b/tests/security/RandomGeneratorTest.php
@@ -14,14 +14,14 @@ class RandomGeneratorTest extends SapphireTest {
public function testGenerateHash() {
$r = new RandomGenerator();
- $this->assertNotNull($r->generateHash());
- $this->assertNotEquals($r->generateHash(), $r->generateHash());
+ $this->assertNotNull($r->randomToken());
+ $this->assertNotEquals($r->randomToken(), $r->randomToken());
}
public function testGenerateHashWithAlgorithm() {
$r = new RandomGenerator();
- $this->assertNotNull($r->generateHash('md5'));
- $this->assertNotEquals($r->generateHash(), $r->generateHash('md5'));
+ $this->assertNotNull($r->randomToken('md5'));
+ $this->assertNotEquals($r->randomToken(), $r->randomToken('md5'));
}
}
diff --git a/tests/security/SecurityTest.php b/tests/security/SecurityTest.php
index f8a566ee4..6337e1b71 100644
--- a/tests/security/SecurityTest.php
+++ b/tests/security/SecurityTest.php
@@ -222,7 +222,12 @@ class SecurityTest extends FunctionalTest {
// 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);
+
+ // We don't have access to the token - generate a new token and hash pair.
+ $token = $admin->generateAutologinTokenAndStoreHash();
+
+ // Check.
+ $response = $this->get('Security/changepassword/?m='.$admin->ID.'&t=' . $token);
$this->assertEquals(302, $response->getStatusCode());
$this->assertEquals(Director::baseUrl() . 'Security/changepassword', $response->getHeader('Location'));