Use password_hash for passwords instead of SHA1

Squashed version of PR #977, with several additional changes: - fix migration improperly removing default null for password column - migrate to ascii_bin collation - move migration ID to current date to avoid later confusion - use PASSWORD_DEFAULT instead of PASSWORD_BCRYPT - simplify/clarify password upgrade checks - upgrade passwords when validating outside login (e.g. when resetting password) - added additional tests for hash upgrades with wrong passwords Co-authored-by: John Flatness <[email protected]>
omeka · Jul 18, 2024 · 14022a5 · 14022a5
1 parent 798e267
commit 14022a5
Show file tree

Hide file tree

Showing 8 changed files with 132 additions and 29 deletions.
diff --git 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
@@ -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
@@ -0,0 +1,20 @@
+<?php
+/**
+ * Omeka
+ *
+ * @copyright Copyright 2007-2022 Roy Rosenzweig Center for History and New Media
+ * @license http://www.gnu.org/licenses/gpl-3.0.txt GNU GPLv3
+ */
+
+/**
+ * Increase length of salt and hash for user passwords
+ *
+ * @package Omeka\Db\Migration
+ */
+class growUserPassword extends Omeka_Db_Migration_AbstractMigration
+{
+    public function up()
+    {
+        $this->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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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')) {