diff --git a/application/libraries/Omeka/Auth/Adapter/UserTable.php b/application/libraries/Omeka/Auth/Adapter/UserTable.php index 9299a7a6b2..bcb85775d1 100644 --- a/application/libraries/Omeka/Auth/Adapter/UserTable.php +++ b/application/libraries/Omeka/Auth/Adapter/UserTable.php @@ -22,7 +22,32 @@ public function __construct(Omeka_Db $db) $db->User, 'username', 'password', - 'SHA1(CONCAT(salt, ?)) AND active = 1'); + 'active = 1'); + } + + /** + * Accept a Zend_Db_Select object and performs a query against + * the database with that object. + * + * Overrides the Zend implementation to check the password using + * password_verify(). + * + * @param Zend_Db_Select $dbSelect + * @throws Zend_Auth_Adapter_Exception - when an invalid select + * object is encountered + * @return array + */ + protected function _authenticateQuerySelect(Zend_Db_Select $dbSelect) + { + $resultIdentities = parent::_authenticateQuerySelect($dbSelect); + $correctResult = array(); + foreach ($resultIdentities as $identity) { + if ($identity['password'] !== null && password_verify($this->_credential, $identity['password'])) { + $identity['zend_auth_credential_match'] = 1; + $correctResult[] = $identity; + } + } + return $correctResult; } /** diff --git a/application/libraries/Omeka/Validate/UserPassword.php b/application/libraries/Omeka/Validate/UserPassword.php index 728ebd4f4d..d260577890 100644 --- a/application/libraries/Omeka/Validate/UserPassword.php +++ b/application/libraries/Omeka/Validate/UserPassword.php @@ -53,7 +53,8 @@ public function __construct(User $user) public function isValid($value, $context = null) { assert($this->_user->password !== null); - $valid = $this->_user->hashPassword($value) === $this->_user->password; + User::upgradeHashedPassword($this->_user->username, $value); + $valid = password_verify($value, $this->_user->password); if (!$valid) { $this->_error(self::INVALID); } diff --git a/application/migrations/20240717204800_growUserPassword.php b/application/migrations/20240717204800_growUserPassword.php new file mode 100644 index 0000000000..108dc80472 --- /dev/null +++ b/application/migrations/20240717204800_growUserPassword.php @@ -0,0 +1,20 @@ +db->query("ALTER TABLE {$this->db->User} MODIFY `password` VARCHAR(255) COLLATE ascii_bin DEFAULT NULL"); + } +} diff --git a/application/models/User.php b/application/models/User.php index 05b984373a..30eadb47b0 100644 --- a/application/models/User.php +++ b/application/models/User.php @@ -33,6 +33,9 @@ class User extends Omeka_Record_AbstractRecord implements Zend_Acl_Resource_Inte /** * The salt for the hashed password. * + * The value "bcrypt" here indicates that $password is a modern PHP + * password_hash hash that already includes the salt + * * @var string */ public $salt; @@ -201,16 +204,27 @@ protected function _validate() * @param string $username * @param string $password * @return bool False if incorrect username/password given, otherwise true - * when password can be or has been upgraded. + * when password has been upgraded. */ public static function upgradeHashedPassword($username, $password) { $userTable = get_db()->getTable('User'); - $user = $userTable->findBySql("username = ? AND salt IS NULL AND password = SHA1(?)", - array($username, $password), true); + $user = $userTable->findBySql("username = ?", array($username), true); if (!$user) { return false; } + // stored password_hash password: doesn't need upgrade + if ($user->salt === 'bcrypt') { + return false; + } + // stored salted SHA1 password, but provided doesn't match + if (!empty($user->salt) && $user->password !== sha1($user->salt . $password)) { + return false; + } + // stored unsalted SHA1 password, but provided doesn't match + if (empty($user->salt) && $user->password !== sha1($password)) { + return false; + } $user->setPassword($password); $user->save(); return true; @@ -243,14 +257,6 @@ public function getResourceId() return 'Users'; } - /** - * Generate a simple 16 character salt for the user. - */ - public function generateSalt() - { - $this->salt = substr(md5(mt_rand()), 0, 16); - } - /** * Set a new password for the user. * @@ -261,20 +267,18 @@ public function generateSalt() */ public function setPassword($password) { - if ($this->salt === null) { - $this->generateSalt(); - } $this->password = $this->hashPassword($password); + $this->salt = 'bcrypt'; } /** - * SHA-1 hash the given password with the current salt. + * Hash the given password * * @param string $password Plain-text password. - * @return string Salted and hashed password. + * @return string Hashed password. */ public function hashPassword($password) { - return sha1($this->salt . $password); + return password_hash($password, PASSWORD_DEFAULT); } } diff --git a/application/schema/users.sql b/application/schema/users.sql index 422c80e4b5..eec7b9b5e7 100644 --- a/application/schema/users.sql +++ b/application/schema/users.sql @@ -3,7 +3,7 @@ CREATE TABLE IF NOT EXISTS `%PREFIX%users` ( `username` varchar(30) collate utf8_unicode_ci NOT NULL, `name` text collate utf8_unicode_ci NOT NULL, `email` text collate utf8_unicode_ci NOT NULL, - `password` varchar(40) collate utf8_unicode_ci default NULL, + `password` varchar(255) collate ascii_bin default NULL, `salt` varchar(16) collate utf8_unicode_ci default NULL, `active` tinyint NOT NULL, `role` varchar(40) collate utf8_unicode_ci NOT NULL default 'default', diff --git a/application/tests/suite/Controllers/ChangePasswordTest.php b/application/tests/suite/Controllers/ChangePasswordTest.php index dbe9e897fe..cdcbb1717d 100644 --- a/application/tests/suite/Controllers/ChangePasswordTest.php +++ b/application/tests/suite/Controllers/ChangePasswordTest.php @@ -130,9 +130,9 @@ private function _assertPasswordNotChanged() private function _assertPasswordIs($pass, $msg = '') { - $this->assertEquals($this->db->fetchOne("SELECT password FROM omeka_users WHERE id = 1"), - $this->user->hashPassword($pass), - $msg); + $this->assertTrue(password_verify($pass, + $this->db->fetchOne("SELECT password FROM omeka_users WHERE id = 1")), + $msg); } private function _assertSaltNotChanged() diff --git a/application/tests/suite/Controllers/LoginTest.php b/application/tests/suite/Controllers/LoginTest.php index 89bf6bf248..7dbff85b85 100644 --- a/application/tests/suite/Controllers/LoginTest.php +++ b/application/tests/suite/Controllers/LoginTest.php @@ -19,15 +19,38 @@ public function testUpgradingHashedPasswordForUser() $dbAdapter = $this->db->getAdapter(); // Reset the username/pass to the old style (SHA1 w/ no salt). $dbAdapter->update('omeka_users', - array('password' => sha1('foobar'), - 'salt' => null), - 'id = 1'); + array('password' => sha1('foobar'), 'salt' => null), + 'id = 1' + ); // Now attempt to login, and verify that the database was upgraded, and - // that the user account was upgraded to use a salt. + // that the user account was upgraded to use bcrypt. $this->_login('foobar123', 'foobar'); $this->assertRedirectTo('/', $this->getResponse()->getBody()); - $this->assertNotNull($dbAdapter->fetchOne("SELECT salt FROM omeka_users WHERE id = 1")); + $newUser = $dbAdapter->fetchRow("SELECT `salt`, `password` FROM omeka_users WHERE id = 1"); + $this->assertNotNull($newUser); + $this->assertEquals($newUser['salt'], 'bcrypt'); + $this->assertTrue(password_verify('foobar', $newUser['password'])); + } + + public function testUpgradingSaltedPasswordForUser() + { + $this->assertTrue($this->db instanceof Omeka_Db); + $dbAdapter = $this->db->getAdapter(); + // Reset the username/pass to the old style (SHA1 w/ salt). + $dbAdapter->update('omeka_users', + array('password' => sha1('0123456789abcdef' . 'barbaz'), 'salt' => '0123456789abcdef'), + 'id = 1' + ); + + // Now attempt to login, and verify that the database was upgraded, and + // that the user account was upgraded to use bcrypt. + $this->_login('foobar123', 'barbaz'); + $this->assertRedirectTo('/', $this->getResponse()->getBody()); + $newUser = $dbAdapter->fetchRow("SELECT `salt`, `password` FROM omeka_users WHERE id = 1"); + $this->assertNotNull($newUser); + $this->assertEquals($newUser['salt'], 'bcrypt'); + $this->assertTrue(password_verify('barbaz', $newUser['password'])); } public function testValidLogin() @@ -43,6 +66,36 @@ public function testInvalidLogin() $this->assertStringContainsString('Login information incorrect. Please try again.', $this->getResponse()->sendResponse()); } + public function testInvalidOldHashedPassword() + { + $this->assertTrue($this->db instanceof Omeka_Db); + $dbAdapter = $this->db->getAdapter(); + // Reset the username/pass to the old style (SHA1 w/ no salt). + $dbAdapter->update('omeka_users', + array('password' => sha1('foobar'), 'salt' => null), + 'id = 1' + ); + + $this->_login('foobar123', 'foobar_not'); + $this->assertNotRedirect(); + $this->assertStringContainsString('Login information incorrect. Please try again.', $this->getResponse()->sendResponse()); + } + + public function testInvalidOldSaltedPassword() + { + $this->assertTrue($this->db instanceof Omeka_Db); + $dbAdapter = $this->db->getAdapter(); + // Reset the username/pass to the old style (SHA1 w/ salt). + $dbAdapter->update('omeka_users', + array('password' => sha1('0123456789abcdef' . 'barbaz'), 'salt' => '0123456789abcdef'), + 'id = 1' + ); + + $this->_login('foobar123', 'foobar_not'); + $this->assertNotRedirect(); + $this->assertStringContainsString('Login information incorrect. Please try again.', $this->getResponse()->sendResponse()); + } + public static function roles() { return array( diff --git a/bootstrap.php b/bootstrap.php index a59e66cf06..bf98a5a8f8 100644 --- a/bootstrap.php +++ b/bootstrap.php @@ -8,7 +8,7 @@ */ // Define the current version of Omeka. -define('OMEKA_VERSION', '3.2-dev2'); +define('OMEKA_VERSION', '3.2-dev3'); // Define the application environment. if (!defined('APPLICATION_ENV')) {