diff --git a/_config.php b/_config.php
index f9e932c73..178ff9bf5 100755
--- a/_config.php
+++ b/_config.php
@@ -66,4 +66,9 @@ define('MCE_ROOT', 'jsparty/tiny_mce/');
*/
define('EMAIL_BOUNCEHANDLER_KEY', '1aaaf8fb60ea253dbf6efa71baaacbb3');
-?>
+PasswordEncryptor::register('none', 'PasswordEncryptor_None');
+PasswordEncryptor::register('md5', 'PasswordEncryptor_LegacyPHPHash("md5")');
+PasswordEncryptor::register('sha1','PasswordEncryptor_LegacyPHPHash("sha1")');
+PasswordEncryptor::register('md5_v2.4', 'PasswordEncryptor_PHPHash("md5")');
+PasswordEncryptor::register('sha1_v2.4','PasswordEncryptor_PHPHash("sha1")');
+?>
\ No newline at end of file
diff --git a/security/Member.php b/security/Member.php
index 5a7ad9faf..ca3d9de06 100644
--- a/security/Member.php
+++ b/security/Member.php
@@ -10,14 +10,18 @@ class Member extends DataObject {
'FirstName' => 'Varchar',
'Surname' => 'Varchar',
'Email' => 'Varchar',
- 'Password' => 'Varchar(64)', // support for up to SHA256!
+ 'Password' => 'Varchar(160)',
'RememberLoginToken' => 'Varchar(50)',
'NumVisit' => 'Int',
'LastVisited' => 'SS_Datetime',
'Bounced' => 'Boolean', // Note: This does not seem to be used anywhere.
'AutoLoginHash' => 'Varchar(30)',
'AutoLoginExpired' => 'SS_Datetime',
- 'PasswordEncryption' => "Enum('none', 'none')",
+ // This is an arbitrary code pointing to a PasswordEncryptor instance,
+ // not an actual encryption algorithm.
+ // Warning: Never change this field after its the first password hashing without
+ // providing a new cleartext password as well.
+ 'PasswordEncryption' => "Varchar(50)",
'Salt' => 'Varchar(50)',
'PasswordExpiry' => 'Date',
'LockedOutUntil' => 'SS_Datetime',
@@ -98,6 +102,9 @@ class Member extends DataObject {
*/
protected static $login_marker_cookie = null;
+ public static function init_db_fields() {
+ }
+
/**
* If this is called, then a session cookie will be set to "1" whenever a user
* logs in. This lets 3rd party tools, such as apache's mod_rewrite, detect
@@ -120,24 +127,6 @@ class Member extends DataObject {
static function set_login_marker_cookie($cookieName) {
self::$login_marker_cookie = $cookieName;
}
-
- /**
- * This method is used to initialize the static database members
- *
- * Since PHP doesn't support any expressions for the initialization of
- * static member variables we need a method that does that.
- *
- * This method adds all supported encryption algorithms to the
- * PasswordEncryption Enum field.
- *
- * @todo Maybe it would be useful to define this in DataObject and call
- * it automatically?
- */
- public static function init_db_fields() {
- self::$db['PasswordEncryption'] = "Enum(array('none', '" .
- implode("', '", array_map("addslashes", Security::get_encryption_algorithms())) .
- "'), 'none')";
- }
/**
* Check if the passed password matches the stored one
@@ -147,10 +136,17 @@ class Member extends DataObject {
*/
public function checkPassword($password) {
// Only confirm that the password matches if the user isn't locked out
- if(!$this->isLockedOut()) {
- $encryption_details = Security::encrypt_password($password, $this->Salt, $this->PasswordEncryption);
- return ($this->Password === $encryption_details['password']);
- }
+ if($this->isLockedOut()) return false;
+
+ $spec = Security::encrypt_password(
+ $password,
+ $this->Salt,
+ $this->PasswordEncryption,
+ $this
+ );
+ $e = $spec['encryptor'];
+
+ return $e->compare($this->Password, $spec['password']);
}
/**
@@ -538,10 +534,18 @@ class Member extends DataObject {
$this->sendInfo('changePassword');
}
- // The test on $this->ID is used for when records are initially created
+ // The test on $this->ID is used for when records are initially created.
+ // Note that this only works with cleartext passwords, as we can't rehash
+ // existing passwords.
if(!$this->ID || $this->isChanged('Password')) {
// Password was changed: encrypt the password according the settings
- $encryption_details = Security::encrypt_password($this->Password);
+ $encryption_details = Security::encrypt_password(
+ $this->Password, // this is assumed to be cleartext
+ $this->Salt,
+ $this->PasswordEncryption,
+ $this
+ );
+ // Overwrite the Password property with the hashed value
$this->Password = $encryption_details['password'];
$this->Salt = $encryption_details['salt'];
$this->PasswordEncryption = $encryption_details['algorithm'];
@@ -1033,6 +1037,12 @@ class Member extends DataObject {
return $valid;
}
+ /**
+ * Change password. This will cause rehashing according to
+ * the `PasswordEncryption` property.
+ *
+ * @param String $password Cleartext password
+ */
function changePassword($password) {
$this->Password = $password;
$valid = $this->validate();
diff --git a/security/MemberPassword.php b/security/MemberPassword.php
index 1063922b2..d53f07693 100644
--- a/security/MemberPassword.php
+++ b/security/MemberPassword.php
@@ -35,11 +35,20 @@ class MemberPassword extends DataObject {
}
/**
- * Check if the given password is the same as the one stored in this record
+ * Check if the given password is the same as the one stored in this record.
+ * See {@link Member->checkPassword()}.
+ *
+ * @param String $password Cleartext password
+ * @return Boolean
*/
function checkPassword($password) {
- $encryption_details = Security::encrypt_password($password, $this->Salt, $this->PasswordEncryption);
- return ($this->Password === $encryption_details['password']);
+ $spec = Security::encrypt_password(
+ $password,
+ $this->Salt,
+ $this->PasswordEncryption
+ );
+ $e = $spec['encryptor'];
+ return $e->compare($this->Password, $spec['password']);
}
diff --git a/security/Security.php b/security/Security.php
index b8f7fb9f0..9627fcb8a 100644
--- a/security/Security.php
+++ b/security/Security.php
@@ -32,21 +32,23 @@ class Security extends Controller {
/**
* Should passwords be stored encrypted?
+ * @deprecated 2.4 Please use 'none' as the default $encryptionAlgorithm instead
*
* @var bool
*/
protected static $encryptPasswords = true;
/**
- * The password encryption algorithm to use if {@link $encryptPasswords}
- * is set to TRUE.
+ * The password encryption algorithm to use by default.
+ * This is an arbitrary code registered through {@link PasswordEncryptor}.
*
* @var string
*/
- protected static $encryptionAlgorithm = 'sha1';
+ protected static $encryptionAlgorithm = 'sha1_v2.4';
/**
* Should a salt be used for the password encryption?
+ * @deprecated 2.4 Please use a custom {@link PasswordEncryptor} instead
*
* @var bool
*/
@@ -703,6 +705,8 @@ class Security extends Controller {
/**
* Set if passwords should be encrypted or not
*
+ * @deprecated 2.4 Use PasswordEncryptor_None instead.
+ *
* @param bool $encrypt Set to TRUE if you want that all (new) passwords
* will be stored encrypted, FALSE if you want to
* store the passwords in clear text.
@@ -713,36 +717,15 @@ class Security extends Controller {
/**
- * Get a list of all available encryption algorithms
+ * Get a list of all available encryption algorithms.
+ * Note: These are arbitrary codes, and not callable methods.
+ *
+ * @deprecated 2.4 Use PasswordEncryptor::get_encryptors()
*
- * @return array Returns an array of strings containing all supported
- * encryption algorithms.
+ * @return array Returns an array of strings containing all supported encryption algorithms.
*/
public static function get_encryption_algorithms() {
- $result = function_exists('hash_algos') ? hash_algos() : array();
-
- if(count($result) == 0) {
- if(function_exists('md5')) $result[] = 'md5';
-
- if(function_exists('sha1')) $result[] = 'sha1';
- } else {
- foreach ($result as $i => $algorithm) {
- if (preg_match('/,/',$algorithm)) {
- unset($result[$i]);
- }
- }
- }
-
- // Support for MySQL password() and old_password() functions. These aren't recommended unless you need them,
- // but can be helpful for migrating legacy user-sets into a SilverStripe application.
- // Since DB::getConn() doesn't exist yet, we need to look at $databaseConfig. Gack!
- global $databaseConfig;
- if($databaseConfig['type'] == 'MySQLDatabase') {
- $result[] = 'password';
- $result[] = 'old_password';
- }
-
- return $result;
+ return array_keys(PasswordEncryptor::get_encryptors());
}
@@ -750,137 +733,75 @@ class Security extends Controller {
* Set the password encryption algorithm
*
* @param string $algorithm One of the available password encryption
- * algorithms determined by
- * {@link Security::get_encryption_algorithms()}
- * @param bool $use_salt Set to TRUE if a random salt should be used to
- * encrypt the passwords, otherwise FALSE
- * @return bool Returns TRUE if the passed algorithm was valid, otherwise
- * FALSE.
+ * algorithms determined by {@link Security::get_encryption_algorithms()}
+ * @return bool Returns TRUE if the passed algorithm was valid, otherwise FALSE.
*/
- public static function set_password_encryption_algorithm($algorithm,
- $use_salt) {
- if(in_array($algorithm, self::get_encryption_algorithms()) == false)
- return false;
-
+ public static function set_password_encryption_algorithm($algorithm) {
+ if(!array_key_exists($algorithm, PasswordEncryptor::get_encryptors())) return false;
+
self::$encryptionAlgorithm = $algorithm;
- self::$useSalt = (bool)$use_salt;
-
return true;
}
-
-
+
/**
- * Get the the password encryption details
- *
- * The return value is an array of the following form:
- *
- * array('encrypt_passwords' => bool,
- * 'algorithm' => string,
- * 'use_salt' => bool)
- *
- *
- * @return array Returns an associative array containing all the
- * password encryption relevant information.
+ * @return String
*/
- public static function get_password_encryption_details() {
- return array('encrypt_passwords' => self::$encryptPasswords,
- 'algorithm' => self::$encryptionAlgorithm,
- 'use_salt' => self::$useSalt);
+ public static function get_password_encryption_algorithm() {
+ return self::$encryptionAlgorithm;
}
-
/**
- * Encrypt a password
- *
- * Encrypt a password according to the current password encryption
- * settings.
- * Use {@link Security::get_password_encryption_details()} to retrieve the
- * current settings.
+ * Encrypt a password according to the current password encryption settings.
* If the settings are so that passwords shouldn't be encrypted, the
* result is simple the clear text password with an empty salt except when
* a custom algorithm ($algorithm parameter) was passed.
*
* @param string $password The password to encrypt
* @param string $salt Optional: The salt to use. If it is not passed, but
- * needed, the method will automatically create a
- * random salt that will then be returned as return
- * value.
+ * needed, the method will automatically create a
+ * random salt that will then be returned as return value.
* @param string $algorithm Optional: Use another algorithm to encrypt the
- * password (so that the encryption algorithm can
- * be changed over the time).
+ * password (so that the encryption algorithm can be changed over the time).
+ * @param Member $member Optional
* @return mixed Returns an associative array containing the encrypted
- * password and the used salt in the form
- * array('encrypted_password' => string, 'salt' =>
- * string, 'algorithm' => string).
- * If the passed algorithm is invalid, FALSE will be
- * returned.
+ * password and the used salt in the form:
+ *
+ * array(
+ * 'password' => string,
+ * 'salt' => string,
+ * 'algorithm' => string,
+ * 'encryptor' => PasswordEncryptor instance
+ * )
+ *
+ * If the passed algorithm is invalid, FALSE will be returned.
*
* @see encrypt_passwords()
* @see set_password_encryption_algorithm()
- * @see get_password_encryption_details()
*/
- public static function encrypt_password($password, $salt = null,
- $algorithm = null) {
- if(strlen(trim($password)) == 0) {
- // An empty password was passed, return an empty password an salt!
- return array('password' => null,
- 'salt' => null,
- 'algorithm' => 'none');
-
- } elseif((!$algorithm && self::$encryptPasswords == false) || ($algorithm == 'none')) {
- // The password should not be encrypted
- return array('password' => substr($password, 0, 64),
- 'salt' => null,
- 'algorithm' => 'none');
-
- } elseif(strlen(trim($algorithm)) != 0) {
- // A custom encryption algorithm was passed, check if we can use it
- if(in_array($algorithm, self::get_encryption_algorithms()) == false)
- return false;
-
+ static function encrypt_password($password, $salt = null, $algorithm = null, $member = null) {
+ if(
+ // if the password is empty, don't encrypt
+ strlen(trim($password)) == 0
+ // if no algorithm is provided and no default is set, don't encrypt
+ || (!$algorithm && self::$encryptPasswords == false)
+ ) {
+ $algorithm = 'none';
} else {
- // Just use the default encryption algorithm
- $algorithm = self::$encryptionAlgorithm;
- }
+ // Fall back to the default encryption algorithm
+ if(!$algorithm) $algorithm = self::$encryptionAlgorithm;
+ }
- // Support for MySQL password() and old_password() authentication
- if(strtolower($algorithm) == 'password' || strtolower($algorithm) == 'old_password') {
- $SQL_password = Convert::raw2sql($password);
- $enc = DB::query("SELECT $algorithm('$SQL_password')")->value();
- return array(
- 'password' => $enc,
- 'salt' => null,
- 'algorithm' => $algorithm,
- );
- }
+ $e = PasswordEncryptor::create_for_algorithm($algorithm);
-
- // If no salt was provided but we need one we just generate a random one
- if(strlen(trim($salt)) == 0)
- $salt = null;
-
- if((self::$useSalt == true) && is_null($salt)) {
- $salt = sha1(mt_rand()) . time();
- $salt = substr(base_convert($salt, 16, 36), 0, 50);
- }
-
-
- // Encrypt the password
- if(function_exists('hash')) {
- $password = hash($algorithm, $password . $salt);
- } else {
- $password = call_user_func($algorithm, $password . $salt);
- }
-
- // Convert the base of the hexadecimal password to 36 to make it shorter
- // In that way we can store also a SHA256 encrypted password in just 64
- // letters.
- $password = substr(base_convert($password, 16, 36), 0, 64);
-
-
- return array('password' => $password,
- 'salt' => $salt,
- 'algorithm' => $algorithm);
+ // New salts will only need to be generated if the password is hashed for the first time
+ $salt = ($salt) ? $salt : $e->salt($password);
+
+ return array(
+ 'password' => $e->encrypt($password, $salt),
+ 'salt' => $salt,
+ 'algorithm' => $algorithm,
+ 'encryptor' => $e
+ );
}
/**
@@ -936,5 +857,4 @@ class Security extends Controller {
}
}
-
-?>
+?>
\ No newline at end of file
diff --git a/tests/security/MemberTest.php b/tests/security/MemberTest.php
index f66630995..f5e0de5b7 100644
--- a/tests/security/MemberTest.php
+++ b/tests/security/MemberTest.php
@@ -6,34 +6,74 @@
class MemberTest extends SapphireTest {
static $fixture_file = 'sapphire/tests/security/MemberTest.yml';
+ function setUp() {
+ parent::setUp();
+
+ Member::set_password_validator(null);
+ }
+
+ function testDefaultPasswordEncryptionOnMember() {
+ $member = new Member();
+ $member->Password = 'mypassword';
+ $member->write();
+ $this->assertEquals(
+ $member->PasswordEncryption,
+ Security::get_password_encryption_algorithm(),
+ 'Password encryption is set for new member records on first write'
+ );
+ }
+
+ function testDefaultPasswordEncryptionDoesntChangeExistingMembers() {
+ $member = new Member();
+ $member->Password = 'mypassword';
+ $member->PasswordEncryption = 'sha1_v2.4';
+ $member->write();
+
+ Security::set_password_encryption_algorithm('none');
+
+ $member->Password = 'mynewpassword';
+ $member->write();
+
+ $this->assertEquals(
+ $member->PasswordEncryption,
+ 'sha1_v2.4'
+ );
+ $this->assertTrue($member->checkPassword("mynewpassword"));
+ }
+
+ function testSetPassword() {
+ $member = $this->objFromFixture('Member', 'test');
+ $member->Password = "test1";
+ $member->write();
+ $this->assertTrue($member->checkPassword("test1"));
+ }
+
/**
* Test that password changes are logged properly
*/
function testPasswordChangeLogging() {
- Member::set_password_validator(null);
-
$member = $this->objFromFixture('Member', 'test');
$this->assertNotNull($member);
$member->Password = "test1";
$member->write();
-
+
$member->Password = "test2";
$member->write();
-
+
$member->Password = "test3";
$member->write();
-
+
$passwords = DataObject::get("MemberPassword", "\"MemberID\" = $member->ID", "\"Created\" DESC, \"ID\" DESC")->getIterator();
$this->assertNotNull($passwords);
$record = $passwords->rewind();
$this->assertTrue($record->checkPassword('test3'), "Password test3 not found in MemberRecord");
-
+
$record = $passwords->next();
$this->assertTrue($record->checkPassword('test2'), "Password test2 not found in MemberRecord");
-
+
$record = $passwords->next();
$this->assertTrue($record->checkPassword('test1'), "Password test1 not found in MemberRecord");
-
+
$record = $passwords->next();
$this->assertType('DataObject', $record);
$this->assertTrue($record->checkPassword('1nitialPassword'), "Password 1nitialPassword not found in MemberRecord");
@@ -44,7 +84,7 @@ class MemberTest extends SapphireTest {
*/
function testChangedPasswordEmaling() {
$this->clearEmails();
-
+
$member = $this->objFromFixture('Member', 'test');
$this->assertNotNull($member);
$valid = $member->changePassword('32asDF##$$%%');
@@ -65,59 +105,59 @@ class MemberTest extends SapphireTest {
$this->assertNotNull($member);
Member::set_password_validator(new NZGovtPasswordValidator());
-
+
// BAD PASSWORDS
$valid = $member->changePassword('shorty');
$this->assertFalse($valid->valid());
$this->assertContains("TOO_SHORT", $valid->codeList());
-
+
$valid = $member->changePassword('longone');
$this->assertNotContains("TOO_SHORT", $valid->codeList());
$this->assertContains("LOW_CHARACTER_STRENGTH", $valid->codeList());
$this->assertFalse($valid->valid());
-
+
$valid = $member->changePassword('w1thNumb3rs');
$this->assertNotContains("LOW_CHARACTER_STRENGTH", $valid->codeList());
$this->assertTrue($valid->valid());
// Clear out the MemberPassword table to ensure that the system functions properly in that situation
DB::query("DELETE FROM \"MemberPassword\"");
-
+
// GOOD PASSWORDS
$valid = $member->changePassword('withSym###Ls');
$this->assertNotContains("LOW_CHARACTER_STRENGTH", $valid->codeList());
$this->assertTrue($valid->valid());
-
+
$valid = $member->changePassword('withSym###Ls2');
$this->assertTrue($valid->valid());
-
+
$valid = $member->changePassword('withSym###Ls3');
$this->assertTrue($valid->valid());
-
+
$valid = $member->changePassword('withSym###Ls4');
$this->assertTrue($valid->valid());
-
+
$valid = $member->changePassword('withSym###Ls5');
$this->assertTrue($valid->valid());
-
+
$valid = $member->changePassword('withSym###Ls6');
$this->assertTrue($valid->valid());
-
+
$valid = $member->changePassword('withSym###Ls7');
$this->assertTrue($valid->valid());
-
+
// CAN'T USE PASSWORDS 2-7, but I can use pasword 1
-
+
$valid = $member->changePassword('withSym###Ls2');
$this->assertFalse($valid->valid());
$this->assertContains("PREVIOUS_PASSWORD", $valid->codeList());
-
+
$valid = $member->changePassword('withSym###Ls5');
$this->assertFalse($valid->valid());
$this->assertContains("PREVIOUS_PASSWORD", $valid->codeList());
-
+
$valid = $member->changePassword('withSym###Ls7');
$this->assertFalse($valid->valid());
$this->assertContains("PREVIOUS_PASSWORD", $valid->codeList());
@@ -126,19 +166,19 @@ class MemberTest extends SapphireTest {
$this->assertTrue($valid->valid());
// HAVING DONE THAT, PASSWORD 2 is now available from the list
-
+
$valid = $member->changePassword('withSym###Ls2');
$this->assertTrue($valid->valid());
-
+
$valid = $member->changePassword('withSym###Ls3');
$this->assertTrue($valid->valid());
-
+
$valid = $member->changePassword('withSym###Ls4');
$this->assertTrue($valid->valid());
-
+
Member::set_password_validator(null);
}
-
+
/**
* Test that the PasswordExpiry date is set when passwords are changed
*/
@@ -152,11 +192,11 @@ class MemberTest extends SapphireTest {
$expiryDate = date('Y-m-d', time() + 90*86400);
$this->assertEquals($expiryDate, $member->PasswordExpiry);
-
+
Member::set_password_expiry(null);
$valid = $member->changePassword("Xx?1234235");
$this->assertTrue($valid->valid());
-
+
$this->assertNull($member->PasswordExpiry);
}
@@ -164,11 +204,11 @@ class MemberTest extends SapphireTest {
$member = $this->objFromFixture('Member', 'test');
$this->assertNotNull($member);
$this->assertFalse($member->isPasswordExpired());
-
+
$member = $this->objFromFixture('Member', 'noexpiry');
$member->PasswordExpiry = null;
$this->assertFalse($member->isPasswordExpired());
-
+
$member = $this->objFromFixture('Member', 'expiredpassword');
$this->assertTrue($member->isPasswordExpired());
@@ -176,7 +216,7 @@ class MemberTest extends SapphireTest {
// If PasswordExpiry == today, then it's expired
$member->PasswordExpiry = date('Y-m-d');
$this->assertTrue($member->isPasswordExpired());
-
+
// If PasswordExpiry == tomorrow, then it's not
$member->PasswordExpiry = date('Y-m-d', time() + 86400);
$this->assertFalse($member->isPasswordExpired());