[AuthCrypt] Password storage and comparison improvements

Password hashes are now stored in a TEXT attribute, not limited to 199 symbols.
That limitation makes no sense as password hashes are not the kind of
information to be indexed.

Actually replace crypt() with password_verify() for password checking, current
code left password_verify() unused.

Only update passwords when they use a different algorithm from the current
default. Previously "overwrite" meant rehashing every login.

Replace the "argon" boolean option with "algorithm" and "algorithm_options" for
better configurability.
The default remains whichever is default for PHP's password_hash.

classes/User.php

@@ -31,7 +31,7 @@ class User extends Managed_DataObject
     public $__table = 'user';                            // table name
     public $id;                              // int(4)  primary_key not_null
     public $nickname;                        // varchar(64)  unique_key
-    public $password;                        // varchar(191)               not 255 because utf8mb4 takes more space
+    public $password;                        // text
     public $email;                           // varchar(191)  unique_key   not 255 because utf8mb4 takes more space
     public $incomingemail;                   // varchar(191)  unique_key   not 255 because utf8mb4 takes more space
     public $emailnotifysub;                  // bool          default_true
@@ -65,7 +65,7 @@ class User extends Managed_DataObject
             'fields' => array(
                 'id' => array('type' => 'int', 'not null' => true, 'description' => 'foreign key to profile table'),
                 'nickname' => array('type' => 'varchar', 'length' => 64, 'description' => 'nickname or username, duped in profile'),
-                'password' => array('type' => 'varchar', 'length' => 191, 'description' => 'salted password, can be null for OpenID users'),
+                'password' => array('type' => 'text', 'description' => 'salted password, can be null for OpenID users'),
                 'email' => array('type' => 'varchar', 'length' => 191, 'description' => 'email address for password recovery etc.'),
                 'incomingemail' => array('type' => 'varchar', 'length' => 191, 'description' => 'email address for post-by-email'),
                 'emailnotifysub' => array('type' => 'bool', 'default' => true, 'description' => 'Notify by email of subscriptions'),

modules/AuthCrypt/AuthCryptModule.php

@@ -15,7 +15,7 @@
 // along with GNU social.  If not, see <http://www.gnu.org/licenses/>.
 
 /**
- * Module to use crypt() for user password hashes
+ * Module to use password_hash() for user password hashes
  *
  * @category  Module
  * @package   GNUsocial
@@ -27,17 +27,44 @@
 
 defined('GNUSOCIAL') || die;
 
+if (version_compare(PHP_VERSION, '7.4.0', '<')) {
+    function password_algos(): array
+    {
+        $algos = [PASSWORD_BCRYPT];
+        defined('PASSWORD_ARGON2I')  && $algos[] = PASSWORD_ARGON2I;
+        defined('PASSWORD_ARGON2ID') && $algos[] = PASSWORD_ARGON2ID;
+        return $algos;
+    }
+}
+
 class AuthCryptModule extends AuthenticationModule
 {
     const MODULE_VERSION = '2.0.0';
-    protected $statusnet = true;     // if true, also check StatusNet style password hash
+    protected $algorithm = PASSWORD_DEFAULT;
+    protected $algorithm_options = [];
     protected $overwrite = true;     // if true, password change means overwrite with crypt()
-    protected $argon     = false;    // Use Argon if supported.
+    protected $statusnet = true;     // if true, also check StatusNet-style password hash
 
     public $provider_name = 'crypt'; // not actually used
 
     // FUNCTIONALITY
 
+    public function onInitializePlugin()
+    {
+        if (!in_array($this->algorithm, password_algos())) {
+            common_log(
+                LOG_ERR,
+                "Unsupported password hashing algorithm: {$this->algorithm}"
+            );
+            $this->algorithm = PASSWORD_DEFAULT;
+        }
+        // Make "'cost' = 12" the default option, but only if bcrypt
+        if ($this->algorithm === PASSWORD_BCRYPT
+            && !array_key_exists('cost', $this->algorithm_options)) {
+            $this->algorithm_options['cost'] = 12;
+        }
+    }
+
     public function checkPassword($username, $password)
     {
         $username = Nickname::normalize($username);
@@ -47,30 +74,30 @@ class AuthCryptModule extends AuthenticationModule
             return false;
         }
 
-        // crypt understands what the salt part of $user->password is
+        $match = false;
-        if ($user->password === crypt($password, $user->password)) {
-            // and update password hash entry to password_hash() compatible
-            if ($this->overwrite) {
-                $this->changePassword($user->nickname, null, $password);
-            }
-            return $user;
-        }
 
-        // If we check StatusNet hash, for backwards compatibility and migration
-        if ($this->statusnet && $user->password === md5($password . $user->id)) {
-            // and update password hash entry to crypt() compatible
-            if ($this->overwrite) {
-                $this->changePassword($user->nickname, null, $password);
-            }
-            return $user;
-        }
-
-        // Timing safe password verification on supported PHP versions
         if (password_verify($password, $user->password)) {
-            return $user;
+            $match = true;
+        } elseif ($this->statusnet) {
+            // Check StatusNet hash, for backwards compatibility and migration
+            // Check size outside regex to take out entries of a differing size faster
+            if (strlen($user->password) === 32
+                && preg_match('/^[a-f0-9]$/D', $user->password)) {
+                $match = hash_equals(
+                    $user->password,
+                    hash('md5', $password . $user->id)
+                );
+            }
         }
 
-        return false;
+        // Update password hash entry if it doesn't match current settings
+        if ($this->overwrite
+            && $match
+            && password_needs_rehash($user->password, $this->algorithm, $this->algorithm_options)) {
+            $this->changePassword($user->nickname, null, $password);
+        }
+
+        return $match ? $user : false;
     }
 
     // $oldpassword is already verified when calling this function... shouldn't this be private?!
@@ -95,21 +122,7 @@ class AuthCryptModule extends AuthenticationModule
 
     public function hashPassword($password, ?Profile $profile = null)
     {
-        $algorithm = PASSWORD_DEFAULT;
+        return password_hash($password, $this->algorithm, $this->algorithm_options);
-        $options = ['cost' => 12];
-
-        if ($this->argon) {
-            $algorithm = PASSWORD_ARGON2I;
-            $options = [
-                'memory_cost' => PASSWORD_ARGON2_DEFAULT_MEMORY_COST,
-                'time_cost' => PASSWORD_ARGON2_DEFAULT_TIME_COST,
-                'threads' => PASSWORD_ARGON2_DEFAULT_THREADS,
-            ];
-        }
-        // Use the modern password hashing algorithm
-        // http://php.net/manual/en/function.password-hash.php
-        // Uses PASSWORD_BCRYPT by default, with PASSWORD_ARGON2I being the next possible default in future versions
-        return password_hash($password, $algorithm, $options);
     }
 
     // EVENTS

modules/AuthCrypt/README.md

@@ -6,12 +6,14 @@ You can change these settings in `config.php` with `$config['AuthCryptModule'][{
 
 Default values in parenthesis.
 
-authoritative (false): Set this to true when _all_ passwords are hashed with crypt()
+authoritative (false): Set this to true when _all_ passwords are hashed with password_hash()
     (warning: this may disable all other password verification, also when changing passwords!)
+algorithm (PASSWORD_DEFAULT): A hashing algorithm to use, the default value is defined by PHP. See all supported strings at https://php.net/password-hash
+algorithm_options (['cost' => 12] if "algorithm" is bcrypt): Hashing algorithm options. See all supported values at https://php.net/password-hash
 statusnet (true): Do we check the password against legacy StatusNet md5 hash?
     (notice: will check password login against old-style hash and if 'overwrite' is enabled update using crypt())
-overwrite (true): Do we overwrite old style password hashes with crypt() hashes on password change?
+overwrite (true): Do we overwrite password hashes on login if they used a different algorithm
-    (notice: to make use of stronger security or migrate to crypt() hashes, this must be true)
+    (notice: to make use of stronger security or migrate obsolete hashes, this must be true)
 password_changeable (true): Enables or disables password changing.
     (notice: if combined with authoritative, it disables changing passwords and removes option from menu.)
 autoregistration: This setting is ignored. Password can never be valid without existing User.