From 7d7dbe627b224deb970d9dcfbe9709516298716f Mon Sep 17 00:00:00 2001 From: Alexei Sorokin Date: Sat, 25 Jul 2020 20:16:21 +0300 Subject: [PATCH] [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 | 4 +- modules/AuthCrypt/AuthCryptModule.php | 89 +++++++++++++++------------ modules/AuthCrypt/README.md | 8 ++- 3 files changed, 58 insertions(+), 43 deletions(-) diff --git a/classes/User.php b/classes/User.php index ebc759779a..142f8ed695 100644 --- a/classes/User.php +++ b/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'), diff --git a/modules/AuthCrypt/AuthCryptModule.php b/modules/AuthCrypt/AuthCryptModule.php index 2c65a8bf84..da4c93bc1f 100644 --- a/modules/AuthCrypt/AuthCryptModule.php +++ b/modules/AuthCrypt/AuthCryptModule.php @@ -15,7 +15,7 @@ // along with GNU social. If not, see . /** - * 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 - 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; - } + $match = false; - // 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; - $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); + return password_hash($password, $this->algorithm, $this->algorithm_options); } // EVENTS diff --git a/modules/AuthCrypt/README.md b/modules/AuthCrypt/README.md index 4932074db3..f15502545a 100644 --- a/modules/AuthCrypt/README.md +++ b/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? - (notice: to make use of stronger security or migrate to crypt() hashes, this must be true) +overwrite (true): Do we overwrite password hashes on login if they used a different algorithm + (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.