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.

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
 	 *

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();

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) {

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());
-	}	
-}
+	}
+
+	/**
+	 * @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);
+	}
+}

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' =>
 					'<p>' . 
@@ -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' => '<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(

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;
 	}	
-}
+}

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 {
 

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'));
 	}
 	
 }

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'));