[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.
This commit is contained in:
Alexei Sorokin
parent
0b947ce2c7
commit
2861ae2823
@ -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'),
|
||||
|
||||
@ -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
|
||||
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
|
||||
|
||||
@ -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.
|
||||
|
||||
Loading…
Reference in New Issue
Block a user