Replace custom password hashing library with PHP password_hash()

Finally managed to manually test a Drupal 6 to Drupal 10.1 (+patch) upgrade. The sequence of steps is analogous to #243. The result is identical as well.

Note: quickly spinning up Drupal 6 isn't that easy nowadays. I went with PHP 5.6 installed from deb.sury.org and a combination of php -S localhost:8888 and drush.phar rs (drush 8). @pcambra points out that ddev should be able to do the job as well.

That said, I think that the PR is ready now. Next step is the change record.

Made a CR draft.

smustgrave → credited mfb → .

Closed 📌 Increase password hashing iterations Closed: duplicate as a duplicate moving over credit

Added some review comments.

Thanks @longwave, addressed.

This is looking pretty good to me. I will raise this issue with the security team regarding a review.

Added an extra comment where one of the deprecation messages has gone a bit wrong.

I think we also need an upgrade path test, to prove that a user created in Drupal 10.0 can still log in in Drupal 10.1 after the phpass module has been enabled.

There also doesn't seem to be a test case that covers the deprecation of \Drupal\Core\Password\PhpassHashedPassword.

Thanks @longwave.

Added an extra comment where one of the deprecation messages has gone a bit wrong.

There also doesn't seem to be a test case that covers the deprecation of \Drupal\Core\Password\PhpassHashedPassword.

Right. Fixed.

I think we also need an upgrade path test, to prove that a user created in Drupal 10.0 can still log in in Drupal 10.1 after the phpass module has been enabled.

This is covered by PasswordVerifyTest::testPasswordCheckSupported(). Do you have something different in mind?

Usually if we have a post_update hook we write an update test that extends UpdatePathTestBase. This ensures that a populated database can successfully upgrade and confirms that the post_update hook works as expected. In this case I think a good test would be to directly set a Phpass password hash before performing the update, and attempt to log in with it following the update.

Also please point me to this if I'm incorrect, but I think we don't have explicit test coverage to ensure passwords hashed in Drupal 10.0 or earlier are upgraded to the new hash format on login? Perhaps this should be part of the same test.

Regarding the security team review, @mlhess and @catch gave me some points to consider. Here are a list of points I came up with; some require further discussion:

1. PHP provides this functionality now, so we should use it - Not Invented Here is almost always better.
2. We are using a modified version of Phpass → , that the original developer of Phpass suggested we should not modify, and who now considers Phpass obsolete and password_hash() should be used instead.
3. In the long run, this change results in less code for us to maintain. However for now we temporarily have to maintain two password hashing libraries, because existing sites can only change hashes when they log in. We probably do not want to maintain legacy passwords forever. When should we consider deprecating the Phpass module and moving it to contrib?
4. #723802: convert to sha-256 and hmac from md5 and sha1 → initially came about because of government requirements around hash algorithms. Are there any such considerations we need to make if we change to password_hash()? I would assume not - but then as far as I have found via Google, bcrypt/Blowfish is not recommended by NIST or other government agencies. NIST appears to recommend PBKDF2 instead. But the docs for hash_pbkdf2() say "The PBKDF2 method can be used for hashing passwords for storage. However, it should be noted that password_hash() or crypt() with CRYPT_BLOWFISH are better suited for password storage." so I am not sure what the correct answer is here.

Regarding #252.3 I just thought of something that might be useful: we could report statistics on how many passwords are hashed with new vs legacy, and present this to the site owner. I think this could be done with a relatively simple database query, given that we only need to check the first three characters of the hash. This would then help indicate whether it is safe to disable the legacy module. Unsure if this is a job for core or contrib, and it shouldn't hold up this issue, but I think it's worth discussing?

Regarding #252.4 I found a discussion at https://news.ycombinator.com/item?id=7286057 which suggests bcrypt is better, but PBKDF2 is referred to in standards, so the question about considerations for government users still applies.

We probably do not want to maintain legacy passwords forever. When should we consider deprecating the Phpass module and moving it to contrib?

I thought about what the criteria for this might be, but hadn't wrote it out yet. At the beginning of writing this comment I didn't have any idea, but writing it out pushed me towards one a bit:

For each site, there is a window between them first installing a version of Drupal that includes the new password hashing algorithm, and when they uninstall the phpass module (or have to install the contrib version explicitly if we've dropped support but they haven't). If that window is two years or five years, then their users just have to log in once during that window to get a new password hash. If they haven't, they'll need to do a password reset. Long login cookie lifetimes could mean that their users don't have to log in for months after they initially update, so it really needs to be years not months to have a chance of catching even most active users on a site.

IMO this is not an easy concept for site owners to understand, much less site visitors - the fact that the hash can only be updated when the user actually logs in and the way this is spread over time, so it'd be quite possible for sites to uninstall the module, lock out their visitors, and not realise what they've done.

For Drupal core / us, there is the point at which we can assume that all Drupal sites should be on 10.1 or higher, this will only be when 10.0, 9.5, and Drupal 7 are out of support. It's very likely that Drupal 7 will be the last of those to go EOL.

Since the old password hashing algorithm is going to be an integral part of the Drupal 7 migration path, then we can't drop it until we move the Drupal 6 and 7 migrations to contrib, and consensus seems to be that this should only happen when Drupal 7 is no longer supported.

However, if we have a major release that supports migrating from Drupal 7, then to allow sites and their users to have a password migration window long enough after they've migrated, I think we should probably consider supporting the old password hashes for one additional major release cycle after the Drupal 7 migration path is moved to contrib.

This would mean:
Drupal 7 site updates to Drupal x
Drupal x+1 is released after Drupal 7 goes EOL.
Drupal x site updates to Drupal x+1 (Phpass still supported in core)
Drupal x+1 site updates to Drupal x+2 (uninstall Phpass or install the contrib version)

That way if you go from Drupal 7 to a mid-release major version (i.e. just before the next major is released or maybe just after that), you still have a solid couple of years or more before we lock out all your users.

I just thought of something that might be useful: we could report statistics on how many passwords are hashed with new vs legacy, and present this to the site owner. I think this could be done with a relatively simple database query, given that we only need to check the first three characters of the hash. This would then help indicate whether it is safe to disable the legacy module.

This is definitely worth a core issue. If we decided not to put it in core, it might be a good thing for upgrade status even?

Related to that, I wonder if we should have a fallback that detects the old password hash fallback when Phpass isn't installed, and when it finds an incompatible hash, tell the user 'You haven't logged in for a while, please reset your password'. Should also be split to its own issue though.

Looks like NIST is currently reviewing SP 800-132. The original document NIST Special Publication 800-132 Recommendation for Password-Based Key Derivation is from 2010. In the meantime Argon2 won the password hashing competition in 2015 and became the recommended hashing algorithm by OWASP.

If I'm not totally mistaken, the current Drupal password hashing algorithm isn't NIST approved neither. My interpretation of #723802: convert to sha-256 and hmac from md5 and sha1 → is that government agencies require the absence of SHA-1 and MD5 (which is reasonable) and not explicitly the presence of PBKDF2. Would be great if somebody with relations to government agencies could verify.

Usually if we have a post_update hook we write an update test that extends UpdatePathTestBase. This ensures that a populated database can successfully upgrade and confirms that the post_update hook works as expected. In this case I think a good test would be to directly set a Phpass password hash before performing the update, and attempt to log in with it following the update.

It looks like UpdatePathTestBaseFilledTest::testUpdatedSite() is doing exactly that (line 97).

I'm going to add a test which reproduces #243 explicitly (including an attempt to try logging in with a user whose password hash hasn't been updated after disabling the phpass module).

Re #257 just for an extra confirmation should we add phpass to the installed module list in that test (lines 346-411)?

This will also need some consideration in Drupal 11 as to how we handle legacy passwords when we create the Drupal 10 dump database for upgrade tests; we should ensure we still have a legacy hash and ensure that it is still correctly managed. This makes me think that perhaps we should write an explicit test for this now?

edit: crosspost with the above

Re #253

I think this could be done with a relatively simple database query, given that we only need to check the first three characters of the hash. This would then help indicate whether it is safe to disable the legacy module. Unsure if this is a job for core or contrib, and it shouldn't hold up this issue, but I think it's worth discussing?

I made a start here: Password Stats. → Its a very simple drush command for the moment.

Not sure what happened with the GitLab comments, I don't seem to be able to dismiss my previous review.

I added three extra comments (the last three above) that need a tiny bit more work, but after that I'm happy to mark this RTBC to get more core committer eyes on it.

Thanks - this has been back and forth a lot now, I have no further comments so marking RTBC for other committers to take a look.

Pretty sure the code locations are inverted and we shouldn't be referencing an module's classes in Core. That would lead to possible fatal errors if referenced and the module wasn't enabled.

Do we have precedent for moving a core service into a module? We want the deprecation error on the core side only.

Maybe we need to temporarily move the code into e.g. \Drupal\Core\Password\LegacyPhpassHashedPassword then \Drupal\Core\Password\PhpassHashedPassword and \Drupal\phpass\Password\PhpassHashedPassword can extend both, with only the former issuing a deprecation, then in Drupal 11 we delete the core classes and move the code to phpass?

Do we have precedent for moving a core service into a module? We want the deprecation error on the core side only.

Yes. The database modules #3129043: Move core database drivers to modules of their own → . I used that as the model.

$ git grep -i trigger_error.*mysql.*E_USER_DEPRECATED
core/lib/Drupal/Core/Database/Driver/mysql/Connection.php:@trigger_error('\Drupal\Core\Database\Driver\mysql\Connection is deprecated in drupal:9.4.0 and is removed from drupal:11.0.0. The MySQL database driver has been moved to the mysql module. See https://www.drupal.org/node/3129492', E_USER_DEPRECATED);
core/lib/Drupal/Core/Database/Driver/mysql/ExceptionHandler.php:@trigger_error('\Drupal\Core\Database\Driver\mysql\ExceptionHandler is deprecated in drupal:9.4.0 and is removed from drupal:11.0.0. The MySQL database driver has been moved to the mysql module. See https://www.drupal.org/node/3129492', E_USER_DEPRECATED);
core/lib/Drupal/Core/Database/Driver/mysql/Insert.php:@trigger_error('\Drupal\Core\Database\Driver\mysql\Insert is deprecated in drupal:9.4.0 and is removed from drupal:11.0.0. The MySQL database driver has been moved to the mysql module. See https://www.drupal.org/node/3129492', E_USER_DEPRECATED);
core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php:@trigger_error('\Drupal\Core\Database\Driver\mysql\Install\Tasks is deprecated in drupal:9.4.0 and is removed from drupal:11.0.0. The MySQL database driver has been moved to the mysql module. See https://www.drupal.org/node/3129492', E_USER_DEPRECATED);
core/lib/Drupal/Core/Database/Driver/mysql/Schema.php:@trigger_error('\Drupal\Core\Database\Driver\mysql\Schema is deprecated in drupal:9.4.0 and is removed from drupal:11.0.0. The MySQL database driver has been moved to the mysql module. See https://www.drupal.org/node/3129492', E_USER_DEPRECATED);
core/lib/Drupal/Core/Database/Driver/mysql/Upsert.php:@trigger_error('\Drupal\Core\Database\Driver\mysql\Upsert is deprecated in drupal:9.4.0 and is removed from drupal:11.0.0. The MySQL database driver has been moved to the mysql module. See https://www.drupal.org/node/3129492', E_USER_DEPRECATED);
core/modules/system/tests/modules/database_statement_monitoring_test/src/mysql/Connection.php:@trigger_error('\Drupal\database_statement_monitoring_test\mysql\Connection is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. There is no replacement. See https://www.drupal.org/node/3318162', E_USER_DEPRECATED);
core/modules/system/tests/modules/database_statement_monitoring_test/src/mysql/Install/Tasks.php:@trigger_error('\Drupal\database_statement_monitoring_test\mysql\Install\Tasks is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. There is no replacement. See https://www.drupal.org/node/3318162', E_USER_DEPRECATED);

Setting this to needs review. There is no obvious next step to work on here.

Due to the critical bug (https://bugs.php.net/bug.php?id=81744) that was fixed in PHP 8.1.16 (https://www.php.net/ChangeLog-8.php#8.1.16), should this require 8.1.16 or newer to avoid the bug?

Weird, the GHSA is rated with severity low. The CVE is still missing most info.

CVE-2023-0567 is 2 commits
- https://github.com/php/php-src/commit/c840f71524067aa474c00c3eacfb83bd86...
- https://github.com/php/php-src/commit/a92acbad873a05470af1a47cb785a18ead...

There is also another GHSA (
DoS vulnerability when parsing multipart request body) which is rated high affecting the same PHP versions. That one is broadly exploitable over the network without authentication.

re #272, I suspect no because of the way OS's versions their PHP versions. Using say Ubuntu's PHP 8.1.13 which will have this patched should be allowed.

My understanding of how we handle language version requirements is to tie them to features that would be required for the site to work. It is up to the site maintainers to keep their OS and language patched against security releases. Don't know of any other security release that's bumped our version requirements so this would be the same.

Re #270 I believe I complained about that too and it _does_ cause fatal errors that are really hard to recover from if you do things incorrectly so I stand behind that being the wrong approach.

> Re #270 I believe I complained about that too and it _does_ cause fatal errors that are really hard to recover from if you do things incorrectly so I stand behind that being the wrong approach.

What would be a reproduction for that case? I'd be interested in testing this out.

smustgrave → credited Jalandhar → .

smustgrave → credited mohit1604 → .

Closed 📌 Add default parameter to PhpassHashedPassword constructor Closed: duplicate in favor of this.

smustgrave → credited borisson_ → .

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased after ✨ Apply SensitiveParameter attribute Fixed . Also added the #[\SensitiveParameter] to PasswordInterface::needsRehash(). That method obviously was missed in the original change.

Re #276, I found one upgrade path which is breaking a standard install. The idea here is to do as much as possible through the web ui and avoid the command line altogether.

• Deploy 10.0.x code base on some PHP hosting.
• Use the web installer to set it up.
• Logout.
• Deploy code base from 1845004-phpass-module branch.
• Attempt to login in order to perform the upgrade.
• Login fails since the post_upgrade hook hasn't had the chance to run.

This update procedure has some other usability problems as well (even without the phpass patch). E.g., the themes loose all their CSS and it is quite hard to navigate the way through to the status site and the upgrade script in the first place.

In order to allow an admin to login after the code base was deployed and before upgrade scripts were run, it is necessary to ensure that the legacy password checking method remains in place for updates sites.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Updated the issue summary in order to match the state of the MR. Addressed #268 by reversing the dependencies (i.e., Drupal\phpass\Password\PhpassHashedPassword now inherits from Drupal\Core\Password\PhpassHashedPasswordBase).

Also introduced a password.core_backward_compat service which wraps the password service on sites in a state where the post update hook had no chance to run yet. This addresses #284. However, the introduction of this service makes it impossible to deprecate Drupal\Core\Password\PhpassHashedPassword in this release cycle. Regrettably it is necessary to postpone the deprecation to the first release which drops support for direct upgrades from Drupal 10.0.x.

This also means that sites newly installed with Drupal 10.1.x will still have the old legacy password hashing class in the code path. Unless they choose to alter away the password.core_backward_compat service, e.g. using a custom or contrib module. Fortunately, new sites will still start out with passwords in PHP password_hash() format for all accounts.

Now that the password.core_backward_compat decorator is in place without any chance for quick deprecation within the current major release cycle, the phpass module is somewhat redundant. Thus, there is some room for minimizing change / complexity here by just dropping the phpass module from this PR. I'd appreciate your thought on this idea.

@znerol

Deploy code base from 1845004-phpass-module branch.
Attempt to login in order to perform the upgrade.
Login fails since the post_upgrade hook hasn't had the chance to run.

This is unsupported, if you want to run updates via the browser without being logged in, you have to set $settings['update_free_access'] = TRUE, so for me it's not a problem with the patch here - any number of things can render browsing a site unusable when it has pending updates to run.

Right, removed password.core_backward_compat again, that makes deprecation easier and more predictable.

Still leaving the @internal abstract base class Drupal\Core\Password\PhpassHashedPasswordBase in tree. So, #268 remains resolved.

Attaching an interdiff of the changes between #265 and #295, which fixes the concerns in #268, to make it simpler for other reviewers. This looks clean to me and I have no further comments.

Also removing the "needs security review" tag; as a member of the security team I believe all concerns that have been raised so far have been addressed adequately, and I have no new concerns to add. Adding "needs followup" for the followup requested by @catch in #255 - we should inform users of when it is safe to uninstall the legacy module, but that can be done after this issue lands.

Marking RTBC as I believe there is no further work to do here, but this is still tagged for framework manager review.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased. Resolved one conflict in core/modules/system/system.post_update.php.

I think this is a good state, but since I've been reviewing it on and off for years, and I'm extremely pro the issue landing because I was extremely opposed to the current password implementation (the phpass fork) that we're deprecating here, I've asked if another framework manager can also review it who's possibly less over-enthusiastic.

+1. Patch applies and passes all the tests.

Passwords still can be null, so added related 🐛 Password is null if user has never logged in which causes PHP 8 warning Fixed

Left a review on the MR - I think we can tighten up the abstract base class to make it narrower and therefore easier to remove once it is no longer needed. We're also doing work that can now be done in a union typehint - something that was not possible when this issue started.

Back to NR.

All review points were addressed, back to RTBC.

I opened 📌 Work out what needs to happen due to core using PHP's password_hash() Active as a follow-up for the contrib module.

Added credit for comments that had impact on the solution, are part of release management and pertained to security issues.

Committed 375376d and pushed to 10.1.x. Thanks!

Added a release note based on the CR.

10 years! congrats!

is the CR is not published for reason?

I published the change notice.

Adjusting issue credit as credit is not provided for button-click rebases without additional contribution.

Automatically closed - issue fixed for 2 weeks with no activity.

Unppostponed follow-up 📌 Deprecate PasswordInterface::check() in favour of ::verify() Active