Permalink
Browse files

API Hash autologin tokens before storing in the database.

Refactor the code to make it clear the distinction is made between a
plaintext token and a hashed version. Rename fields so it is more
obvious what is being written and what sent out to the user.

This reuses the salt and algorithm from the Member, which are kept
constant throughout the Member lifetime in a normal scenario. If they do
change, users will need to re-request so the hashes can be regenerated.
  • Loading branch information...
mateusz authored and chillu committed Nov 8, 2012
1 parent 896ce60 commit a8b0e44d980eeea708dabcab66a8ae605baeb9de
View
@@ -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 @@ public function logIn($remember = false) {
$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 @@ public static function autoLogin() {
$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 @@ public static function autoLogin() {
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 @@ public function logOut() {
$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 @@ public static function member_from_autologinhash($RAW_hash, $login = false) {
return $member;
}
-
/**
* Send signup, change password or forgot password informations to an user
*
@@ -258,12 +258,12 @@ public function forgotPassword($data) {
$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();
@@ -99,7 +99,7 @@ public static function create_for_algorithm($algorithm) {
*/
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 @@ public function checkAEncryptionLevel() {
*/
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) {
@@ -58,15 +58,31 @@ public function generateEntropy() {
// 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());
- }
-}
+ }
+
+ /**
+ * @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);
+ }
+}
View
@@ -532,15 +532,20 @@ public function passwordsent($request) {
/**
- * 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 @@ public function changepassword() {
$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' =>
'<p>' .
@@ -584,16 +596,16 @@ public function changepassword() {
'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' => '<p>'
. _t('Security.CHANGEPASSWORDBELOW', 'You can change your password below.') . '</p>',
'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(
@@ -227,7 +227,7 @@ public function isEnabled() {
*/
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 @@ public function setValue($val) {
public function generate() {
return null;
}
-}
+}
@@ -624,6 +624,35 @@ protected function removeExtensions($extensions) {
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();
+
+ $m1 = new Member();
+ $m1->PasswordEncryption = 'blowfish';
+ $m1->Salt = $enc->salt('123');
+ $m1Token = $m1->generateAutologinTokenAndStoreHash();
+
+ $m2 = new Member();
+ $m2->PasswordEncryption = 'blowfish';
+ $m2->Salt = $enc->salt('456');
+ $m2Token = $m2->generateAutologinTokenAndStoreHash();
+
+ $this->assertTrue($m1->validateAutoLoginToken($m1Token), 'Passes token validity test against matching member.');
+ $this->assertFalse($m2->validateAutoLoginToken($m1Token), 'Fails token validity test against other member.');
+ }
+
}
class MemberTest_ViewingAllowedExtension extends DataExtension implements TestOnly {
@@ -14,14 +14,14 @@ public function testGenerateEntropy() {
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'));
}
}
@@ -222,7 +222,12 @@ public function testChangePasswordFromLostPassword() {
// 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'));

0 comments on commit a8b0e44

Please sign in to comment.