diff --git a/core/core.services.yml b/core/core.services.yml index 5c42b23..2dcdbc9 100644 --- a/core/core.services.yml +++ b/core/core.services.yml @@ -41,6 +41,7 @@ parameters: exposedHeaders: false maxAge: false supportsCredentials: false + password_hash_cost: 10 services: # Simple cache contexts, directly derived from the request context. cache_context.ip: @@ -914,15 +915,20 @@ services: class: Drupal\Core\Path\PathValidator arguments: ['@router', '@router.no_access_checks', '@current_user', '@path_processor_manager'] -# The argument to the hashing service defined in services.yml, to the -# constructor of PhpassHashedPassword is the log2 number of iterations for -# password stretching. -# @todo increase by 1 every Drupal version in order to counteract increases in -# the speed and power of computers available to crack the hashes. The current -# password hashing method was introduced in Drupal 7 with a log2 count of 15. + # The first argument of the hashing service (constructor of PhpPassword) is + # the 'cost' option of password_hash(). In Drupal 8 the 'cost' has the default + # value used by password_hash() which is 10. Future versions of Drupal may + # increase this value in order to counteract increases in the speed and power + # of computers available to crack the hashes. Note that an increase of 1 will + # double the time needed for password hashing. password: - class: Drupal\Core\Password\PhpassHashedPassword + class: Drupal\Core\Password\PhpPassword + arguments: ['%password_hash_cost%', '@legacy_password'] + lazy: true + legacy_password: + class: Drupal\Core\Password\LegacyPassword arguments: [16] + lazy: true request_format_route_filter: class: Drupal\Core\Routing\RequestFormatRouteFilter tags: diff --git a/core/lib/Drupal/Core/Password/PhpassHashedPassword.php b/core/lib/Drupal/Core/Password/LegacyPassword.php similarity index 94% copy from core/lib/Drupal/Core/Password/PhpassHashedPassword.php copy to core/lib/Drupal/Core/Password/LegacyPassword.php index 28313d2..0a2b281 100644 --- a/core/lib/Drupal/Core/Password/PhpassHashedPassword.php +++ b/core/lib/Drupal/Core/Password/LegacyPassword.php @@ -5,12 +5,16 @@ use Drupal\Component\Utility\Crypt; /** - * Secure password hashing functions based on the Portable PHP password - * hashing framework. + * Legacy password hashing framework for Drupal 7 and Drupal < 8.3.0. + * + * A custom password hashing mechanism used in Drupal 7 and < 8.3.0, used to + * validate passwords hashed in Drupal < 8.3.0 or passwords migrated from Drupal + * 6, 7 that are being rehashed on first user login. * * @see http://www.openwall.com/phpass/ */ -class PhpassHashedPassword implements PasswordInterface { +class LegacyPassword extends PasswordHashBase { + /** * The minimum allowed log2 number of iterations for password stretching. */ @@ -154,7 +158,7 @@ protected function enforceLog2Boundaries($count_log2) { */ protected function crypt($algo, $password, $setting) { // Prevent DoS attacks by refusing to hash large passwords. - if (strlen($password) > PasswordInterface::PASSWORD_MAX_LENGTH) { + if (strlen($password) > PasswordHashInterface::PASSWORD_MAX_LENGTH) { return FALSE; } @@ -218,7 +222,7 @@ public function hash($password) { /** * {@inheritdoc} */ - public function check($password, $hash) { + public function verify($password, $hash) { if (substr($hash, 0, 2) == 'U$') { // This may be an updated password from user_update_7000(). Such hashes // have 'U' added as the first character and need an extra md5() (see the diff --git a/core/lib/Drupal/Core/Password/PasswordInterface.php b/core/lib/Drupal/Core/Password/PasswordInterface.php index b38cc79..a671eed 100644 --- a/core/lib/Drupal/Core/Password/PasswordInterface.php +++ b/core/lib/Drupal/Core/Password/PasswordInterface.php @@ -4,6 +4,14 @@ /** * Secure password hashing functions for user authentication. + * + * @deprecated Scheduled for removal in Drupal 9.0.x. Constants and properties + * will me moved in \Drupal\Core\Password\PasswordHashInterface. Use + * \Drupal\Core\Password\PasswordHashInterface instead. + * + * @see \Drupal\Core\Password\PasswordHashInterface + * + * @todo Merge this interface into PasswordHashInterface in Drupal 9.0.x. */ interface PasswordInterface { @@ -33,21 +41,31 @@ public function hash($password); * * @return bool * TRUE if the password is valid, FALSE if not. + * + * @deprecated Scheduled for removal in Drupal 9.0.x. Implement interface + * PasswordHashInterface instead of PasswordInterface and use + * \Drupal\Core\Password\PasswordHashInterface::verify(). + * + * @see \Drupal\Core\Password\PasswordHashInterface::verify() */ public function check($password, $hash); /** * Check whether a hashed password needs to be replaced with a new hash. * - * This is typically called during the login process when the plain text - * password is available. A new hash is needed when the desired iteration - * count has changed by a modification of the password-service in the - * dependency injection container or if the user's password hash was - * generated in an update like user_update_7000() (see the Drupal 7 - * documentation). + * This is typically called during the login process in order to trigger the + * rehashing of the password, as in that stage, the plain text password is + * available. + * + * This method returns TRUE in one of the next cases: + * - The parameters of hashing service were changed. For instance, the hashing + * cost parameter 'password_hash_cost' was changed in core.services.yml. + * - The password hash was hashed in Drupal < 8.3.0. + * - The hash was migrated from a system that uses MD5 hashes, like Drupal 6, + * or from Drupal 7. * * @param string $hash - * The existing hash to be checked. + * The hash to be checked. * * @return bool * TRUE if the hash is outdated and needs rehash. diff --git a/core/lib/Drupal/Core/Password/PhpPassword.php b/core/lib/Drupal/Core/Password/PhpPassword.php new file mode 100644 index 0000000..ca43b7d --- /dev/null +++ b/core/lib/Drupal/Core/Password/PhpPassword.php @@ -0,0 +1,99 @@ +=5.5.0 password hashing. + * + * @see http://php.net/manual/en/book.password.php + */ +class PhpPassword extends PasswordHashBase { + + /** + * The algorithmic cost that should be used. + * + * This is the same 'cost' option as is used by the PHP (>= 5.5.0) + * password_hash() function. + * + * @var int + * + * @see password_hash(). + * @see http://php.net/manual/en/ref.password.php + */ + protected $cost; + + /** + * The legacy password hashing service. + * + * This password hashing service was used in Drupal 7 and Drupal < 8.3.0. + * + * @var \Drupal\Core\Password\PasswordHashInterface + */ + protected $legacyPassword; + + /** + * Constructs a new password hashing instance. + * + * @param int $cost + * The algorithmic cost that should be used. + * @param \Drupal\Core\Password\PasswordHashInterface $legacy_password + * The legacy password hashing service. + */ + function __construct($cost, PasswordHashInterface $legacy_password) { + $this->cost = $cost; + $this->legacyPassword = $legacy_password; + } + + /** + * {@inheritdoc} + */ + public function hash($password) { + // Prevent DoS attacks by refusing to hash large passwords. + if (strlen($password) > static::PASSWORD_MAX_LENGTH) { + return FALSE; + } + + return password_hash($password, PASSWORD_BCRYPT, $this->getOptions()); + } + + /** + * {@inheritdoc} + */ + public function verify($password, $hash) { + // Drupal >= 8.3.x hashed password. + if (substr($hash, 0, 4) === '$2y$') { + $stored_hash = $hash; + } + // Drupal 6 (or any md5) hashed password migrated to Drupal >= 8.3.x. + elseif (substr($hash, 0, 5) === 'U$2y$') { + $stored_hash = substr($hash, 1); + $password = md5($password); + } + // Possible legacy hash. This may be: + // - Either a Drupal 7, < 8.3.0 hash, + // - Or a Drupal 6 (or md5) hash migrated to Drupal < 8.3.0. + else { + return $this->legacyPassword->verify($password, $hash); + } + + return password_verify($password, $stored_hash); + } + + /** + * {@inheritdoc} + */ + public function needsRehash($hash) { + return password_needs_rehash($hash, PASSWORD_BCRYPT, $this->getOptions()); + } + + /** + * Returns password options. + * + * @return array + * Associative array with password options. + */ + protected function getOptions() { + return ['cost' => $this->cost]; + } + +} diff --git a/core/lib/Drupal/Core/Password/PhpassHashedPassword.php b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php index 28313d2..71811dd 100644 --- a/core/lib/Drupal/Core/Password/PhpassHashedPassword.php +++ b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php @@ -2,267 +2,10 @@ namespace Drupal\Core\Password; -use Drupal\Component\Utility\Crypt; - /** - * Secure password hashing functions based on the Portable PHP password - * hashing framework. + * Deprecated legacy password hashing framework. * - * @see http://www.openwall.com/phpass/ + * @deprecated Scheduled for removal in Drupal 9.0.x. Use + * \Drupal\Core\Password\LegacyPassword instead. */ -class PhpassHashedPassword implements PasswordInterface { - /** - * The minimum allowed log2 number of iterations for password stretching. - */ - const MIN_HASH_COUNT = 7; - - /** - * The maximum allowed log2 number of iterations for password stretching. - */ - const MAX_HASH_COUNT = 30; - - /** - * The expected (and maximum) number of characters in a hashed password. - */ - const HASH_LENGTH = 55; - - /** - * Returns a string for mapping an int to the corresponding base 64 character. - */ - public static $ITOA64 = './0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz'; - - /** - * Specifies the number of times the hashing function will be applied when - * generating new password hashes. The number of times is calculated by - * raising 2 to the power of the given value. - */ - protected $countLog2; - - /** - * Constructs a new password hashing instance. - * - * @param int $countLog2 - * Password stretching iteration count. Specifies the number of times the - * hashing function will be applied when generating new password hashes. - * The number of times is calculated by raising 2 to the power of the given - * value. - */ - function __construct($countLog2) { - // Ensure that $countLog2 is within set bounds. - $this->countLog2 = $this->enforceLog2Boundaries($countLog2); - } - - /** - * Encodes bytes into printable base 64 using the *nix standard from crypt(). - * - * @param string $input - * The string containing bytes to encode. - * @param int $count - * The number of characters (bytes) to encode. - * - * @return string - * Encoded string. - */ - protected function base64Encode($input, $count) { - $output = ''; - $i = 0; - do { - $value = ord($input[$i++]); - $output .= static::$ITOA64[$value & 0x3f]; - if ($i < $count) { - $value |= ord($input[$i]) << 8; - } - $output .= static::$ITOA64[($value >> 6) & 0x3f]; - if ($i++ >= $count) { - break; - } - if ($i < $count) { - $value |= ord($input[$i]) << 16; - } - $output .= static::$ITOA64[($value >> 12) & 0x3f]; - if ($i++ >= $count) { - break; - } - $output .= static::$ITOA64[($value >> 18) & 0x3f]; - } while ($i < $count); - - return $output; - } - - /** - * Generates a random base 64-encoded salt prefixed with hash settings. - * - * Proper use of salts may defeat a number of attacks, including: - * - The ability to try candidate passwords against multiple hashes at once. - * - The ability to use pre-hashed lists of candidate passwords. - * - The ability to determine whether two users have the same (or different) - * password without actually having to guess one of the passwords. - * - * @return string - * A 12 character string containing the iteration count and a random salt. - */ - protected function generateSalt() { - $output = '$S$'; - // We encode the final log2 iteration count in base 64. - $output .= static::$ITOA64[$this->countLog2]; - // 6 bytes is the standard salt for a portable phpass hash. - $output .= $this->base64Encode(Crypt::randomBytes(6), 6); - return $output; - } - - /** - * Ensures that $count_log2 is within set bounds. - * - * @param int $count_log2 - * Integer that determines the number of iterations used in the hashing - * process. A larger value is more secure, but takes more time to complete. - * - * @return int - * Integer within set bounds that is closest to $count_log2. - */ - protected function enforceLog2Boundaries($count_log2) { - if ($count_log2 < static::MIN_HASH_COUNT) { - return static::MIN_HASH_COUNT; - } - elseif ($count_log2 > static::MAX_HASH_COUNT) { - return static::MAX_HASH_COUNT; - } - - return (int) $count_log2; - } - - /** - * Hash a password using a secure stretched hash. - * - * By using a salt and repeated hashing the password is "stretched". Its - * security is increased because it becomes much more computationally costly - * for an attacker to try to break the hash by brute-force computation of the - * hashes of a large number of plain-text words or strings to find a match. - * - * @param string $algo - * The string name of a hashing algorithm usable by hash(), like 'sha256'. - * @param string $password - * Plain-text password up to 512 bytes (128 to 512 UTF-8 characters) to - * hash. - * @param string $setting - * An existing hash or the output of $this->generateSalt(). Must be at least - * 12 characters (the settings and salt). - * - * @return string - * A string containing the hashed password (and salt) or FALSE on failure. - * The return string will be truncated at HASH_LENGTH characters max. - */ - protected function crypt($algo, $password, $setting) { - // Prevent DoS attacks by refusing to hash large passwords. - if (strlen($password) > PasswordInterface::PASSWORD_MAX_LENGTH) { - return FALSE; - } - - // The first 12 characters of an existing hash are its setting string. - $setting = substr($setting, 0, 12); - - if ($setting[0] != '$' || $setting[2] != '$') { - return FALSE; - } - $count_log2 = $this->getCountLog2($setting); - // Stored hashes may have been crypted with any iteration count. However we - // do not allow applying the algorithm for unreasonable low and high values - // respectively. - if ($count_log2 != $this->enforceLog2Boundaries($count_log2)) { - return FALSE; - } - $salt = substr($setting, 4, 8); - // Hashes must have an 8 character salt. - if (strlen($salt) != 8) { - return FALSE; - } - - // Convert the base 2 logarithm into an integer. - $count = 1 << $count_log2; - - // We rely on the hash() function being available in PHP 5.2+. - $hash = hash($algo, $salt . $password, TRUE); - do { - $hash = hash($algo, $hash . $password, TRUE); - } while (--$count); - - $len = strlen($hash); - $output = $setting . $this->base64Encode($hash, $len); - // $this->base64Encode() of a 16 byte MD5 will always be 22 characters. - // $this->base64Encode() of a 64 byte sha512 will always be 86 characters. - $expected = 12 + ceil((8 * $len) / 6); - return (strlen($output) == $expected) ? substr($output, 0, static::HASH_LENGTH) : FALSE; - } - - /** - * Parses the log2 iteration count from a stored hash or setting string. - * - * @param string $setting - * An existing hash or the output of $this->generateSalt(). Must be at least - * 12 characters (the settings and salt). - * - * @return int - * The log2 iteration count. - */ - public function getCountLog2($setting) { - return strpos(static::$ITOA64, $setting[3]); - } - - /** - * {@inheritdoc} - */ - public function hash($password) { - return $this->crypt('sha512', $password, $this->generateSalt()); - } - - /** - * {@inheritdoc} - */ - public function check($password, $hash) { - if (substr($hash, 0, 2) == 'U$') { - // This may be an updated password from user_update_7000(). Such hashes - // have 'U' added as the first character and need an extra md5() (see the - // Drupal 7 documentation). - $stored_hash = substr($hash, 1); - $password = md5($password); - } - else { - $stored_hash = $hash; - } - - $type = substr($stored_hash, 0, 3); - switch ($type) { - case '$S$': - // A normal Drupal 7 password using sha512. - $computed_hash = $this->crypt('sha512', $password, $stored_hash); - break; - case '$H$': - // phpBB3 uses "$H$" for the same thing as "$P$". - case '$P$': - // A phpass password generated using md5. This is an - // imported password or from an earlier Drupal version. - $computed_hash = $this->crypt('md5', $password, $stored_hash); - break; - default: - return FALSE; - } - - // Compare using hashEquals() instead of === to mitigate timing attacks. - return $computed_hash && Crypt::hashEquals($stored_hash, $computed_hash); - } - - /** - * {@inheritdoc} - */ - public function needsRehash($hash) { - // Check whether this was an updated password. - if ((substr($hash, 0, 3) != '$S$') || (strlen($hash) != static::HASH_LENGTH)) { - return TRUE; - } - // Ensure that $count_log2 is within set bounds. - $count_log2 = $this->enforceLog2Boundaries($this->countLog2); - // Check whether the iteration count used differs from the standard number. - return ($this->getCountLog2($hash) !== $count_log2); - } - -} +class PhpassHashedPassword extends LegacyPassword { } diff --git a/core/lib/Drupal/Core/ProxyClass/Password/LegacyPassword.php b/core/lib/Drupal/Core/ProxyClass/Password/LegacyPassword.php new file mode 100644 index 0000000..711c23a --- /dev/null +++ b/core/lib/Drupal/Core/ProxyClass/Password/LegacyPassword.php @@ -0,0 +1,104 @@ +container = $container; + $this->drupalProxyOriginalServiceId = $drupal_proxy_original_service_id; + } + + /** + * Lazy loads the real service from the container. + * + * @return object + * Returns the constructed real service. + */ + protected function lazyLoadItself() + { + if (!isset($this->service)) { + $this->service = $this->container->get($this->drupalProxyOriginalServiceId); + } + + return $this->service; + } + + /** + * {@inheritdoc} + */ + public function getCountLog2($setting) + { + return $this->lazyLoadItself()->getCountLog2($setting); + } + + /** + * {@inheritdoc} + */ + public function hash($password) + { + return $this->lazyLoadItself()->hash($password); + } + + /** + * {@inheritdoc} + */ + public function check($password, $hash) + { + return $this->lazyLoadItself()->check($password, $hash); + } + + /** + * {@inheritdoc} + */ + public function needsRehash($hash) + { + return $this->lazyLoadItself()->needsRehash($hash); + } + + } + +} diff --git a/core/lib/Drupal/Core/ProxyClass/Password/PhpPassword.php b/core/lib/Drupal/Core/ProxyClass/Password/PhpPassword.php new file mode 100644 index 0000000..f5cdaab --- /dev/null +++ b/core/lib/Drupal/Core/ProxyClass/Password/PhpPassword.php @@ -0,0 +1,96 @@ +container = $container; + $this->drupalProxyOriginalServiceId = $drupal_proxy_original_service_id; + } + + /** + * Lazy loads the real service from the container. + * + * @return object + * Returns the constructed real service. + */ + protected function lazyLoadItself() + { + if (!isset($this->service)) { + $this->service = $this->container->get($this->drupalProxyOriginalServiceId); + } + + return $this->service; + } + + /** + * {@inheritdoc} + */ + public function hash($password) + { + return $this->lazyLoadItself()->hash($password); + } + + /** + * {@inheritdoc} + */ + public function check($password, $hash) + { + return $this->lazyLoadItself()->check($password, $hash); + } + + /** + * {@inheritdoc} + */ + public function needsRehash($hash) + { + return $this->lazyLoadItself()->needsRehash($hash); + } + + } + +} diff --git a/core/modules/simpletest/src/KernelTestBase.php b/core/modules/simpletest/src/KernelTestBase.php index fbf58a5..38f4185 100644 --- a/core/modules/simpletest/src/KernelTestBase.php +++ b/core/modules/simpletest/src/KernelTestBase.php @@ -364,8 +364,10 @@ public function containerBuild(ContainerBuilder $container) { $definition->clearTag('path_processor_inbound')->clearTag('path_processor_outbound'); } - if ($container->hasDefinition('password')) { - $container->getDefinition('password')->setArguments(array(1)); + // Relax the password hashing cost in tests to avoid performance issues. + if ($container->hasDefinition('password') && $container->hasDefinition('legacy_password')) { + $container->getDefinition('legacy_password')->setArguments([1]); + $container->getDefinition('password')->setArguments([4, $container->get('legacy_password')]); } // Register the stream wrapper manager. diff --git a/core/modules/user/src/Plugin/migrate/destination/EntityUser.php b/core/modules/user/src/Plugin/migrate/destination/EntityUser.php index b11d867..1050a7a 100644 --- a/core/modules/user/src/Plugin/migrate/destination/EntityUser.php +++ b/core/modules/user/src/Plugin/migrate/destination/EntityUser.php @@ -8,7 +8,7 @@ use Drupal\Core\Entity\EntityStorageInterface; use Drupal\Core\Field\FieldTypePluginManagerInterface; use Drupal\Core\Field\Plugin\Field\FieldType\EmailItem; -use Drupal\Core\Password\PasswordInterface; +use Drupal\Core\Password\PasswordHashInterface; use Drupal\migrate\Plugin\MigrationInterface; use Drupal\migrate\Plugin\migrate\destination\EntityContentBase; use Drupal\migrate\Row; @@ -24,7 +24,7 @@ class EntityUser extends EntityContentBase { /** * The password service class. * - * @var \Drupal\Core\Password\PasswordInterface + * @var \Drupal\Core\Password\PasswordHashInterface */ protected $password; @@ -47,10 +47,10 @@ class EntityUser extends EntityContentBase { * The entity manager service. * @param \Drupal\Core\Field\FieldTypePluginManagerInterface $field_type_manager * The field type plugin manager service. - * @param \Drupal\Core\Password\PasswordInterface $password + * @param \Drupal\Core\Password\PasswordHashInterface $password * The password service. */ - public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, EntityStorageInterface $storage, array $bundles, EntityManagerInterface $entity_manager, FieldTypePluginManagerInterface $field_type_manager, PasswordInterface $password) { + public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, EntityStorageInterface $storage, array $bundles, EntityManagerInterface $entity_manager, FieldTypePluginManagerInterface $field_type_manager, PasswordHashInterface $password) { parent::__construct($configuration, $plugin_id, $plugin_definition, $migration, $storage, $bundles, $entity_manager, $field_type_manager); $this->password = $password; } diff --git a/core/modules/user/src/Tests/UserLoginTest.php b/core/modules/user/src/Tests/UserLoginTest.php index af00c74..8941bdb 100644 --- a/core/modules/user/src/Tests/UserLoginTest.php +++ b/core/modules/user/src/Tests/UserLoginTest.php @@ -106,39 +106,118 @@ function testPerUserLoginFloodControl() { } /** - * Test that user password is re-hashed upon login after changing $count_log2. + * Tests that user password is re-hashed upon login, after changing the cost. */ - function testPasswordRehashOnLogin() { - // Determine default log2 for phpass hashing algorithm - $default_count_log2 = 16; - - // Retrieve instance of password hashing algorithm - $password_hasher = $this->container->get('password'); + function testPasswordRehashOnLoginAfterChangingCost() { + /** @var \Drupal\Core\Password\PasswordHashInterface $hashing_service */ + $hashing_service = $this->container->get('password'); // Create a new user and authenticate. - $account = $this->drupalCreateUser(array()); - $password = $account->pass_raw; + $account = $this->drupalCreateUser(); + $plain_password = $account->pass_raw; + $old_hash = $account->getPassword(); + + // Check that the stored password doesn't need rehash. + $this->assertFalse($hashing_service->needsRehash($old_hash)); + + // The current hashing cost is set to 10 in the container. Increase cost by + // one, by enabling a module containing the necessary container changes. + \Drupal::service('module_installer')->install(['user_custom_pass_hash_params_test']); + $this->resetAll(); + // Reload the hashing service after container changes. + $hashing_service = $this->container->get('password'); + + // Check that the stored password needs rehash. + $this->assertTrue($hashing_service->needsRehash($old_hash)); + + // User first login after the password hashing cost was changed. $this->drupalLogin($account); $this->drupalLogout(); - // Load the stored user. The password hash should reflect $default_count_log2. - $user_storage = $this->container->get('entity.manager')->getStorage('user'); - $account = User::load($account->id()); - $this->assertIdentical($password_hasher->getCountLog2($account->getPassword()), $default_count_log2); - - // Change the required number of iterations by loading a test-module - // containing the necessary container builder code and then verify that the - // users password gets rehashed during the login. - $overridden_count_log2 = 19; - \Drupal::service('module_installer')->install(array('user_custom_phpass_params_test')); - $this->resetAll(); - $account->pass_raw = $password; + // Check that after login the password has been rehashed and is valid. + $new_hash = User::load($account->id())->getPassword(); + $this->assertNotEqual($new_hash, $old_hash); + $this->assertTrue($hashing_service->verify($plain_password, $new_hash)); + $this->assertFalse($hashing_service->needsRehash($new_hash)); + } + + /** + * Tests rehashing of Drupal 6 (md5) passwords migrated to Drupal 8. + */ + public function testDrupal6MigratedPasswordRehashing() { + /** @var \Drupal\Core\Password\PasswordHashInterface $main_hashing_service */ + $main_hashing_service = $this->container->get('password'); + + $account = $this->drupalCreateUser(); + $plain_password = $account->pass_raw; + $md5_pass = md5($plain_password); + + /** @var \Drupal\Core\Password\PasswordHashInterface[] $migration_cases */ + $migration_cases = [ + // Drupal 6 (md5) passwords migrated to Drupal < 8.3.0 used the legacy + // password hashing engine, inherited from Drupal 7. + $this->container->get('legacy_password'), + // Drupal 6 (md5) passwords migrated to Drupal >= 8.3.0 are using the + // current hashing password engine, based PHP >=5.5.0 password hashing. + $main_hashing_service, + ]; + + foreach ($migration_cases as $hashing_service) { + // The user has been migrate from Drupal 6. The Drupal 6 md5 hashed + // password was rehashed with 'password' or 'legacy_password' password + // hashing service and prefixed with 'U'. + $old_hash = 'U' . $hashing_service->hash($md5_pass); + // Store the hashed password, but prevent rehashing. + $account->setPassword($old_hash); + $account->get('pass')->pre_hashed = TRUE; + $account->save(); + + // Check that the migrated password needs rehash. + $this->assertTrue($hashing_service->needsRehash($old_hash)); + + // User first login after migration. + $this->drupalLogin($account); + $this->drupalLogout(); + + // Check that after login the password has been rehashed and is valid. + $new_hash = User::load($account->id())->getPassword(); + $this->assertNotEqual($new_hash, $old_hash); + $this->assertTrue($main_hashing_service->verify($plain_password, $new_hash)); + $this->assertFalse($main_hashing_service->needsRehash($new_hash)); + } + } + + /** + * Tests rehashing of Drupal 7 and < 8.3.0 passwords. + */ + public function testPasswordRehashing() { + /** @var \Drupal\Core\Password\PasswordHashInterface $hashing_service */ + $hashing_service = $this->container->get('password'); + /** @var \Drupal\Core\Password\PasswordHashInterface $legacy_hashing_service */ + $legacy_hashing_service = $this->container->get('legacy_password'); + + $account = $this->drupalCreateUser(); + $plain = $account->pass_raw; + + // User has Drupal < 8.3.0 hashed password or migrated from Drupal 7. + $old_hash = $legacy_hashing_service->hash($plain); + // Store the password but prevent rehashing. + $account->setPassword($old_hash); + $account->get('pass')->pre_hashed = TRUE; + $account->save(); + + // Check that the migrated password needs rehashing with the new service. + $this->assertTrue($hashing_service->needsRehash($old_hash)); + + // User first login after changing the hashing service. $this->drupalLogin($account); - // Load the stored user, which should have a different password hash now. - $user_storage->resetCache(array($account->id())); - $account = $user_storage->load($account->id()); - $this->assertIdentical($password_hasher->getCountLog2($account->getPassword()), $overridden_count_log2); - $this->assertTrue($password_hasher->check($password, $account->getPassword())); + $this->drupalLogout(); + + // Check that after login the password has been rehashed and is valid. + $new_hash = User::load($account->id())->getPassword(); + $this->assertNotEqual($new_hash, $old_hash); + $this->assertTrue($hashing_service->verify($plain, $new_hash)); + $this->assertFalse($hashing_service->needsRehash($new_hash)); } /** diff --git a/core/modules/user/src/UserAuth.php b/core/modules/user/src/UserAuth.php index 9fbcf09..d18eed3 100644 --- a/core/modules/user/src/UserAuth.php +++ b/core/modules/user/src/UserAuth.php @@ -3,7 +3,7 @@ namespace Drupal\user; use Drupal\Core\Entity\EntityManagerInterface; -use Drupal\Core\Password\PasswordInterface; +use Drupal\Core\Password\PasswordHashInterface; /** * Validates user authentication credentials. @@ -20,7 +20,7 @@ class UserAuth implements UserAuthInterface { /** * The password hashing service. * - * @var \Drupal\Core\Password\PasswordInterface + * @var \Drupal\Core\Password\PasswordHashInterface */ protected $passwordChecker; @@ -29,10 +29,10 @@ class UserAuth implements UserAuthInterface { * * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager * The entity manager. - * @param \Drupal\Core\Password\PasswordInterface $password_checker + * @param \Drupal\Core\Password\PasswordHashInterface $password_checker * The password service. */ - public function __construct(EntityManagerInterface $entity_manager, PasswordInterface $password_checker) { + public function __construct(EntityManagerInterface $entity_manager, PasswordHashInterface $password_checker) { $this->entityManager = $entity_manager; $this->passwordChecker = $password_checker; } @@ -47,7 +47,7 @@ public function authenticate($username, $password) { $account_search = $this->entityManager->getStorage('user')->loadByProperties(array('name' => $username)); if ($account = reset($account_search)) { - if ($this->passwordChecker->check($password, $account->getPassword())) { + if ($this->passwordChecker->verify($password, $account->getPassword())) { // Successful authentication. $uid = $account->id(); diff --git a/core/modules/user/tests/modules/user_custom_pass_hash_params_test/user_custom_pass_hash_params_test.info.yml b/core/modules/user/tests/modules/user_custom_pass_hash_params_test/user_custom_pass_hash_params_test.info.yml new file mode 100644 index 0000000..aca50c4 --- /dev/null +++ b/core/modules/user/tests/modules/user_custom_pass_hash_params_test/user_custom_pass_hash_params_test.info.yml @@ -0,0 +1,6 @@ +name: 'User custom password hash params test' +type: module +description: 'Support module for testing custom hashing password algorithm parameters.' +package: Testing +version: VERSION +core: 8.x diff --git a/core/modules/user/tests/modules/user_custom_pass_hash_params_test/user_custom_pass_hash_params_test.services.yml b/core/modules/user/tests/modules/user_custom_pass_hash_params_test/user_custom_pass_hash_params_test.services.yml new file mode 100644 index 0000000..82595d1 --- /dev/null +++ b/core/modules/user/tests/modules/user_custom_pass_hash_params_test/user_custom_pass_hash_params_test.services.yml @@ -0,0 +1,7 @@ +services: + password: + class: Drupal\Core\Password\PhpPassword + arguments: [11, '@legacy_password'] + legacy_password: + class: Drupal\Core\Password\LegacyPassword + arguments: [16] diff --git a/core/modules/user/tests/modules/user_custom_phpass_params_test/user_custom_phpass_params_test.info.yml b/core/modules/user/tests/modules/user_custom_phpass_params_test/user_custom_phpass_params_test.info.yml deleted file mode 100644 index 68b8ff9..0000000 --- a/core/modules/user/tests/modules/user_custom_phpass_params_test/user_custom_phpass_params_test.info.yml +++ /dev/null @@ -1,6 +0,0 @@ -name: 'User custom phpass params test' -type: module -description: 'Support module for testing custom phpass password algorithm parameters.' -package: Testing -version: VERSION -core: 8.x diff --git a/core/modules/user/tests/modules/user_custom_phpass_params_test/user_custom_phpass_params_test.services.yml b/core/modules/user/tests/modules/user_custom_phpass_params_test/user_custom_phpass_params_test.services.yml deleted file mode 100644 index 8950bfe..0000000 --- a/core/modules/user/tests/modules/user_custom_phpass_params_test/user_custom_phpass_params_test.services.yml +++ /dev/null @@ -1,4 +0,0 @@ -services: - password: - class: Drupal\Core\Password\PhpassHashedPassword - arguments: [19] diff --git a/core/modules/user/tests/src/Unit/UserAuthTest.php b/core/modules/user/tests/src/Unit/UserAuthTest.php index 1b96d8f..23e2885 100644 --- a/core/modules/user/tests/src/Unit/UserAuthTest.php +++ b/core/modules/user/tests/src/Unit/UserAuthTest.php @@ -2,6 +2,7 @@ namespace Drupal\Tests\user\Unit; +use Drupal\Core\Password\PasswordHashInterface; use Drupal\Tests\UnitTestCase; use Drupal\user\UserAuth; @@ -21,7 +22,7 @@ class UserAuthTest extends UnitTestCase { /** * The mocked password service. * - * @var \Drupal\Core\Password\PasswordInterface|\PHPUnit_Framework_MockObject_MockObject + * @var \Drupal\Core\Password\PasswordHashInterface|\PHPUnit_Framework_MockObject_MockObject */ protected $passwordService; @@ -65,7 +66,7 @@ protected function setUp() { ->with('user') ->will($this->returnValue($this->userStorage)); - $this->passwordService = $this->getMock('Drupal\Core\Password\PasswordInterface'); + $this->passwordService = $this->getMock(PasswordHashInterface::class); $this->testUser = $this->getMockBuilder('Drupal\user\Entity\User') ->disableOriginalConstructor() diff --git a/core/tests/Drupal/KernelTests/KernelTestBase.php b/core/tests/Drupal/KernelTests/KernelTestBase.php index 4da96ba..2730622 100644 --- a/core/tests/Drupal/KernelTests/KernelTestBase.php +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php @@ -624,9 +624,10 @@ public function register(ContainerBuilder $container) { ->clearTag('path_processor_outbound'); } - if ($container->hasDefinition('password')) { - $container->getDefinition('password') - ->setArguments(array(1)); + // Relax the password hashing cost in tests to avoid performance issues. + if ($container->hasDefinition('password') && $container->hasDefinition('legacy_password')) { + $container->getDefinition('legacy_password')->setArguments([1]); + $container->getDefinition('password')->setArguments([4, $container->get('legacy_password')]); } TestServiceProvider::addRouteProvider($container); } diff --git a/core/tests/Drupal/Tests/Core/Password/PasswordHashingTest.php b/core/tests/Drupal/Tests/Core/Password/PasswordHashingTest.php index 5c378ed..8ce4430 100644 --- a/core/tests/Drupal/Tests/Core/Password/PasswordHashingTest.php +++ b/core/tests/Drupal/Tests/Core/Password/PasswordHashingTest.php @@ -2,143 +2,176 @@ /** * @file - * Contains \Drupal\Tests\Core\Password\PasswordHashingTest. + * Contains \Drupal\Tests\Core\Password\PasswordTest. */ namespace Drupal\Tests\Core\Password; -use Drupal\Core\Password\PhpassHashedPassword; -use Drupal\Core\Password\PasswordInterface; +use Drupal\Core\Password\LegacyPassword; +use Drupal\Core\Password\PasswordHashInterface; +use Drupal\Core\Password\PhpPassword; use Drupal\Tests\UnitTestCase; /** - * Unit tests for password hashing API. + * Tests password hashing services. * - * @coversDefaultClass \Drupal\Core\Password\PhpassHashedPassword + * @coversDefaultClass \Drupal\Core\Password\PhpPassword * @group System */ class PasswordHashingTest extends UnitTestCase { /** - * The user for testing. + * The current password hashing service. * - * @var \PHPUnit_Framework_MockObject_MockObject|\Drupal\user\UserInterface + * @var \Drupal\Core\Password\PasswordHashInterface */ - protected $user; + protected $hashingService; /** - * The raw password. + * The legacy hashing service. + * + * This service was used in Drupal 7 and Drupal < 8.3.0. + * + * @var \Drupal\Core\Password\PasswordHashInterface + */ + protected $legacyHashingService; + + /** + * The plain-text password. * * @var string */ - protected $password; + protected $plainPassword; /** - * The md5 password. + * A Drupal 6 (md5) hash migrated with legacy hashing service. + * + * This is a string migrated from Drupal 6 (or any system with md5 hashing) + * either to Drupal 7 or to Drupal < 8.3.0. Such a string is build by hashing + * an already md5 hashed password with the legacy service (used in Drupal 7, + * < 8.3.0) and prefixed with 'U'. + * + * @var string + */ + protected $md5ToLegacyHashedPassword; + + /** + * A Drupal 6 (md5) hash migrated with current hashing service. + * + * This is a string migrated from Drupal 6 (or any system with md5 hashing) to + * Drupal >= 8.3.0. Such a string is build by hashing an already md5 hashed + * password with the current service (used in Drupal >= 8.3.0) password and + * prefixed with 'U'. * * @var string */ protected $md5HashedPassword; /** - * The hashed password. + * A plain password hashed with the legacy service. + * + * This is a plain-text password hashed with the legacy hashing service, used + * in Drupal 7 and Drupal < 8.3.0. * * @var string */ - protected $hashedPassword; + protected $legacyHashedPassword; /** - * The password hasher under test. + * A plain password hashed with the current service. + * + * This is a plain-text password hashed with the current hashing service, used + * Drupal >= 8.3.0. * - * @var \Drupal\Core\Password\PhpassHashedPassword + * @var string */ - protected $passwordHasher; + protected $hashedPassword; /** * {@inheritdoc} */ protected function setUp() { parent::setUp(); - $this->password = $this->randomMachineName(); - $this->passwordHasher = new PhpassHashedPassword(1); - $this->hashedPassword = $this->passwordHasher->hash($this->password); - $this->md5HashedPassword = 'U' . $this->passwordHasher->hash(md5($this->password)); - } - /** - * Tests the hash count boundaries are enforced. - * - * @covers ::enforceLog2Boundaries - */ - public function testWithinBounds() { - $hasher = new FakePhpassHashedPassword(); - $this->assertEquals(PhpassHashedPassword::MIN_HASH_COUNT, $hasher->enforceLog2Boundaries(1), "Min hash count enforced"); - $this->assertEquals(PhpassHashedPassword::MAX_HASH_COUNT, $hasher->enforceLog2Boundaries(100), "Max hash count enforced"); - } + $this->legacyHashingService = new LegacyPassword(1); + $this->hashingService = new PhpPassword(4, $this->legacyHashingService); + + $this->plainPassword = $this->randomMachineName(); + $md5_hash = md5($this->plainPassword); + $this->md5ToLegacyHashedPassword = 'U' . $this->legacyHashingService->hash($md5_hash); + $this->md5HashedPassword = 'U' . $this->hashingService->hash($md5_hash); + $this->legacyHashedPassword = $this->legacyHashingService->hash($this->plainPassword); + $this->hashedPassword = $this->hashingService->hash($this->plainPassword); + } /** - * Test a password needs update. + * Tests if a password needs rehashing. * - * @covers ::needsRehash + * @covers \Drupal\Core\Password\PhpPassword::needsRehash */ - public function testPasswordNeedsUpdate() { - // The md5 password should be flagged as needing an update. - $this->assertTrue($this->passwordHasher->needsRehash($this->md5HashedPassword), 'Upgraded md5 password hash needs a new hash.'); + public function testPasswordNeedsRehashing() { + // Check that outdated hashes need rehashing. + $this->assertTrue($this->hashingService->needsRehash($this->md5ToLegacyHashedPassword)); + $this->assertTrue($this->hashingService->needsRehash($this->md5HashedPassword)); + $this->assertTrue($this->hashingService->needsRehash($this->legacyHashedPassword)); + + // Check that a text hashed with the current service doesn't need rehashing. + $this->assertFalse($this->hashingService->needsRehash($this->hashedPassword)); } /** - * Test password hashing. + * Tests that plain-text password is verifying against all its hashes. * - * @covers ::hash - * @covers ::getCountLog2 - * @covers ::check - * @covers ::needsRehash + * @covers \Drupal\Core\Password\PhpPassword::verify + * @covers \Drupal\Core\Password\LegacyPassword::verify */ public function testPasswordHashing() { - $this->assertSame($this->passwordHasher->getCountLog2($this->hashedPassword), PhpassHashedPassword::MIN_HASH_COUNT, 'Hashed password has the minimum number of log2 iterations.'); - $this->assertNotEquals($this->hashedPassword, $this->md5HashedPassword, 'Password hashes not the same.'); - $this->assertTrue($this->passwordHasher->check($this->password, $this->md5HashedPassword), 'Password check succeeds.'); - $this->assertTrue($this->passwordHasher->check($this->password, $this->hashedPassword), 'Password check succeeds.'); - // Since the log2 setting hasn't changed and the user has a valid password, - // userNeedsNewHash() should return FALSE. - $this->assertFalse($this->passwordHasher->needsRehash($this->hashedPassword), 'Does not need a new hash.'); + // Check that text hashed with current service is different than teh others. + $this->assertNotEquals($this->hashedPassword, $this->md5ToLegacyHashedPassword); + $this->assertNotEquals($this->hashedPassword, $this->md5HashedPassword); + $this->assertNotEquals($this->hashedPassword, $this->legacyHashedPassword); + + // Check that the plain-text password is verifying against all its hashes. + // This is important because migrated and legacy hashes should be checked + // with the user plain-text entered password on first login. + $this->assertTrue($this->hashingService->verify($this->plainPassword, $this->md5ToLegacyHashedPassword)); + $this->assertTrue($this->hashingService->verify($this->plainPassword, $this->md5HashedPassword)); + $this->assertTrue($this->hashingService->verify($this->plainPassword, $this->legacyHashedPassword)); } /** - * Tests password rehashing. + * Tests that password needs rehashing when the cost changes. * - * @covers ::hash - * @covers ::getCountLog2 - * @covers ::check - * @covers ::needsRehash + * @covers \Drupal\Core\Password\PhpPassword::hash + * @covers \Drupal\Core\Password\PhpPassword::verify + * @covers \Drupal\Core\Password\PhpPassword::needsRehash */ - public function testPasswordRehashing() { - // Increment the log2 iteration to MIN + 1. - $password_hasher = new PhpassHashedPassword(PhpassHashedPassword::MIN_HASH_COUNT + 1); - $this->assertTrue($password_hasher->needsRehash($this->hashedPassword), 'Needs a new hash after incrementing the log2 count.'); + public function testPasswordNeedsRehashingOnCostChange() { + // Increment the cost from 4 to 5. + $this->hashingService = new PhpPassword(5, $this->legacyHashingService); + + // Check that the hash needs rehashing after cost changes. + $this->assertTrue($this->hashingService->needsRehash($this->hashedPassword)); + // Re-hash the password. - $rehashed_password = $password_hasher->hash($this->password); - $this->assertSame($password_hasher->getCountLog2($rehashed_password), PhpassHashedPassword::MIN_HASH_COUNT + 1, 'Re-hashed password has the correct number of log2 iterations.'); - $this->assertNotEquals($rehashed_password, $this->hashedPassword, 'Password hash changed again.'); - - // Now the hash should be OK. - $this->assertFalse($password_hasher->needsRehash($rehashed_password), 'Re-hashed password does not need a new hash.'); - $this->assertTrue($password_hasher->check($this->password, $rehashed_password), 'Password check succeeds with re-hashed password.'); - $this->assertTrue($this->passwordHasher->check($this->password, $rehashed_password), 'Password check succeeds with re-hashed password with original hasher.'); + $rehashed_password = $this->hashingService->hash($this->plainPassword); + $this->assertNotEquals($rehashed_password, $this->hashedPassword); + + // Check that the new hash is up-to-date. + $this->assertFalse($this->hashingService->needsRehash($rehashed_password)); + $this->assertTrue($this->hashingService->check($this->plainPassword, $rehashed_password)); } /** - * Verifies that passwords longer than 512 bytes are not hashed. + * Tests that passwords longer than 512 bytes are not hashed. * - * @covers ::crypt + * @covers \Drupal\Core\Password\PhpPassword::hash * * @dataProvider providerLongPasswords */ public function testLongPassword($password, $allowed) { - - $hashed_password = $this->passwordHasher->hash($password); - + $hashed_password = $this->hashingService->hash($password); if ($allowed) { $this->assertNotFalse($hashed_password); } @@ -148,34 +181,48 @@ public function testLongPassword($password, $allowed) { } /** - * Provides the test matrix for testLongPassword(). + * Provides the test cases for testLongPassword(). + * + * @see ::testLongPassword() */ public function providerLongPasswords() { // '512 byte long password is allowed.' - $passwords['allowed'] = array(str_repeat('x', PasswordInterface::PASSWORD_MAX_LENGTH), TRUE); + $passwords['allowed'] = [str_repeat('x', PasswordHashInterface::PASSWORD_MAX_LENGTH), TRUE]; // 513 byte long password is not allowed. - $passwords['too_long'] = array(str_repeat('x', PasswordInterface::PASSWORD_MAX_LENGTH + 1), FALSE); + $passwords['too_long'] = [str_repeat('x', PasswordHashInterface::PASSWORD_MAX_LENGTH + 1), FALSE]; // Check a string of 3-byte UTF-8 characters, 510 byte long password is // allowed. - $len = floor(PasswordInterface::PASSWORD_MAX_LENGTH / 3); - $diff = PasswordInterface::PASSWORD_MAX_LENGTH % 3; - $passwords['utf8'] = array(str_repeat('€', $len), TRUE); + $len = floor(PasswordHashInterface::PASSWORD_MAX_LENGTH / 3); + $diff = PasswordHashInterface::PASSWORD_MAX_LENGTH % 3; + $passwords['utf8'] = [str_repeat('€', $len), TRUE]; // 512 byte long password is allowed. - $passwords['ut8_extended'] = array($passwords['utf8'][0] . str_repeat('x', $diff), TRUE); + $passwords['ut8_extended'] = [$passwords['utf8'][0] . str_repeat('x', $diff), TRUE]; // Check a string of 3-byte UTF-8 characters, 513 byte long password is // allowed. - $passwords['utf8_too_long'] = array(str_repeat('€', $len + 1), FALSE); + $passwords['utf8_too_long'] = [str_repeat('€', $len + 1), FALSE]; + return $passwords; } + /** + * Tests if legacy service hash count boundaries are enforced. + * + * @covers \Drupal\Core\Password\LegacyPassword::enforceLog2Boundaries + */ + public function testWithinBounds() { + $legacy_service = new FakeLegacyPassword(); + $this->assertEquals(LegacyPassword::MIN_HASH_COUNT, $legacy_service->enforceLog2Boundaries(1)); + $this->assertEquals(LegacyPassword::MAX_HASH_COUNT, $legacy_service->enforceLog2Boundaries(100)); + } + } /** - * A fake class for tests. + * A fake legacy hashing class service for tests. */ -class FakePhpassHashedPassword extends PhpassHashedPassword { +class FakeLegacyPassword extends LegacyPassword { function __construct() { // Noop.