Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#21022 Reintroduce support for passwords hashed with MD5 #8414

Closed
wants to merge 2 commits into from

Conversation

johnbillion
Copy link
Member

@johnbillion johnbillion commented Feb 26, 2025

Prior to r59828, passwords hashed with plain md5 were still supported when checking passwords, and were opportunistically rehashed using phpass when a user successfully logged in. Removing this support isn't necessary in order to make the switch to bcrypt, it was done in order to remove this legacy support since md5 hasn't been used to hash passwords since WordPress 2.5 was released in 2008 (see Core-2394).

However, inserting or updating a user password as an md5 hash -- either directly in an SQL query or via a database management tool such as phpMyAdmin -- is not only a useful means of helping a user get back into a site that they might otherwise be locked out of, but it's also a widely documented method. As much as we might not like it, twenty years worth of tutorials and web host help pages exist online that tell users to use this method when the standard password reset process cannot be used (for example when outbound email isn't working, or the user's email address is no longer working, no longer accessible, long forgotten, owned by someone else, etc). There has been some discussion about this internally in the security team and also in the comments on the announcement post.

This PR reinstates the ability for a user to log in to an account where the password is hashed using md5. It will get opportunistically upgraded to bcrypt at the point where a user successfully logs in, just like one gets upgraded to phpass in WP 6.7 and earlier. This does not affect the security of the bcrypt or phpass implementations.

Steps to test

  1. Forget your password.
  2. Use a database management tool to run a query that resets your password to one that uses an md5 hash:
    UPDATE wp_users
    SET user_pass = md5('password')
    WHERE ID = <id>
  3. Confirm that you can log in using your chosen password
  4. Confirm that the user_pass field for your user has been upgraded to a bcrypt hash (with the $wp$2y$ prefix)
  5. Log out and confirm you can still log in with the same password

Trac ticket: https://core.trac.wordpress.org/ticket/21022

Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@johnbillion johnbillion marked this pull request as ready for review February 26, 2025 19:44
Copy link

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props johnbillion.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Contributor

@audrasjb audrasjb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright the logic looks good to me, thanks.

Copy link
Member

@aaronjorbin aaronjorbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggestion/question, but otherwise this looks good to me


if ( ! empty( $wp_hasher ) ) {
// Check the hash using md5 regardless of the current hashing mechanism.
$check = hash_equals( $hash, md5( $password ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since hash_equals requires a string as the first argument, would it make sense to add a check or force it to be a string here?

@@ -363,8 +363,6 @@ public function test_wp_check_password_does_not_support_empty_password( $value )

public function data_empty_values() {
return array(
// Integer zero:
array( 0 ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we force $hash in wp_check_password to be a string, this can be restored.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, although I'm not sure why I added that test value in the first place (it was only introduced in r59828). There is no code path in core that will result in something other than a string being passed to either the $password or $hash parameters of wp_check_password(), and both are documented as strings.

My preference would be to not add a string check or string coercion to the function, and allow PHP to continue triggering the appropriate warning depending on what is passed. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections from me

@johnbillion
Copy link
Member Author

Committed in r59893

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants