Make WordPress Core

Opened 5 years ago

Closed 13 days ago

#50027 closed defect (bug) (duplicate)

Retire Phpass and use PHP native password hashing

Reported by: ayeshrajans's profile ayeshrajans Owned by: johnbillion's profile johnbillion
Milestone: Priority: normal
Severity: normal Version:
Component: Security Keywords: 2nd-opinion needs-unit-tests needs-patch
Focuses: Cc:

Description

PHP comes with built-in password hashing functions since PHP 5.5. Now that we have updated the minimum requirements to PHP 5.6, we can rely on PHP to provide us with password hashing mechanisms that ensures a cryptographically secure random numbers are are used for salt, and the hashes are backwards compatible.

I created and maintain PHP Native Password Hash plugin to swap WordPress's baked in Phpass with PHPs.

0.Phpass recommends to use PHP native hashing

At this time, if your new project can afford to require PHP 5.5+, which it should, please use PHP's native password_hash() / password_verify() API instead of phpass.

I propose that we upgrade the hashing mechanisms to password_hash()/password_verify/password_needs_rehash() combo.

1.We do not need to force users to change their passwords.

Phpass-hashed passwords have the signature $P, and the very old MD5 hashes are fewer than 32 characters long. We will inspect the signature first, and if the password is using the old standard, we will validate the password one last-time, and then use password_hash() to rehash it. From this point forward, that user is "upgraded" to the new mechanism.

2.Expose a filter for plugins

The plugin I maintain supports BCrypt, Argon2I, and Argon2ID for hashing. We can expose a filter that WordPress core emits so plugins can change the hashing algorithm if necessary.

3.Use BCrypt as the default algorithm

If a plugin does not take over, WordPress core will use BCrypt. BCrypt is secure, and is available in any PHP version 5.5, 5.6, 7.* and 8.*.

4.Do not remove Phpass

We will not remove Phpass from WordPress core. This is needed for backwards compatibility to ensure that existing users will eventually be updated.

The end goal is that we seamlessly migrate active users passwords to better mechanisms without breaking functionality for existing users. Frameworks such as Drupal and phpBB (which used phpass in the past) have moved to better mechanisms since the minimum required PHP versions have been updated, and we can easily follow suit.

If the maintainers agree, I would be overjoyed to collaborate on patches.

Change History (21)

#1 @ayeshrajans
5 years ago

Related #21022: A ticket opened 8 years ago, and last reply 7 months ago. The conversation drives to use password_hash() when minimum required PHP version is upped to 5.6

Related #39499: A suggestion to use Argon2ID (which is quite secure no doubt), but requires to polyfill it with a quite slow user-land polyfill. It was not well-received and I don't suggest to use sodium_compat either.

#2 @SergeyBiryukov
5 years ago

  • Component changed from General to Security

#3 @bgermann
5 years ago

The last patch version on #21022 has all your requirements implemented except for "2.Expose a filter for plugins".

#4 @Otto42
5 years ago

@ayeshrajans Looking at your plugin, you are hashing the raw password, which has the problem of truncating it at 72 characters, as was still being discussed over in #21022.

Other than that, creating a patch is relatively straightforward. Filters could be added to allow changes to the $algo and $options parameters of password_hash, however those should be left as the default settings in most/all cases.

The only issue remaining for a patch is the 72 character limitation.

One of the solutions suggested in the previous ticket is to essentially do this to the password:

$prehash = base64_encode( hash( 'sha512', $raw_password, true ) )

And then to run the prehash through password_hash. The purpose of this is to reduce the size of the password from whatever length it is down to small enough to fit into the limit for password_hash. This is always better than truncation, really.

The reason for the base64 is speed, simplicity, and avoiding null bytes in the hash. These are valid reasons.

Minor problem is that sha512 produces a 64 byte result, and base64'ing it produces an 88 byte string. Slightly longer than what we want. Better than nothing even with truncation, but still.

A viable alternative that ticks all the boxes would be to use sha384 instead.
@paragoninitiativeenterprises : please weigh in on this idea:

This produces a 64 byte string. Always:

$prehash = base64_encode( hash( 'sha384', $raw_password, true ) )

It's not sha512. But the purpose of using a hash here is to reduce the possibly very long passphrase down into a reasonable length. The substantial problem and difference in using various hashes here is thus the possibility of collision. There is a higher collision chance with 384 vs. 512. However, given that the longer 512 result would be truncated by a whole 16 bytes anyway (which is equivalent to 12 bytes in the 4/3 conversion from base64), then the difference is not particularly significant.

As for support, I believe the sha384 algorithm should be equally available in all php versions where sha512 exists.

So, I suggest the following:

a) Switch all password hashing to password_hash() using $prehash = base64_encode( hash( 'sha384', $raw_password, true ) ) as a size reducer to allow for "unlimited" password length.

b) Due to speed of hashing considerations and DOS attacks, prefilter the allowed password length down to 4096 characters, the same as we already have in WordPress and for the exact same reasons. This code will remain essentially the same.

c) @mbijon suggested using batch processing and usermeta. This is not necessary, nor actually possible. The conversion process would need the original password input by the user, so it can only be done at the time they actually login, same as the old MD5 upgrade process. So, we modify the existing process to upgrade from old $P passwords to new ones in the exact same manner as it currently does for MD5 hashes.

d) Centralize the use of password_hash (and password_verify), so that filters can be applied to it across the board, in case somebody wants to change $algo or $options.

e) For obvious reasons, the phpass library will need to remain to verify existing hashes before they are upgraded. No plans to remove or deprecate it are necessary. We still convert old MD5 hashes, after all.

Given all that, a patch to implement this is relatively straightforward, if we can agree on step A and using a prehash method to overcome the 72 character truncation problem of password_hash.

I suggest sha384. Any objections?

#5 follow-up: @ayeshrajans
5 years ago

Thanks a lot for the in-depth comment @Otto42, very helpful.

In that plugin, one of the use cases was to make it possible for WordPress and a custom site to share login, so having raw passwords gone through password_hash helped to keep things cleaner, so it became a design choice to ignore the 72 char limit.

I see the points raised about bcrypt's 72 byte limitation, and while they are valid and can solve the very problem of "unlimited" length passwords, I would like to propose that we do not pre-hash passwords.

Can we simplify this? We have two approaches:

  • Ignore that bcrypt truncates beyond 72 bytes. Hard-reject at 4096 bytes regardless of the hashing algorithm (bcrypt, argon2, etc). This introduces no BC backwards, and will allow us to accept longer passwords (So correct horse battery staple-akin passwords are allowed). As for security, I would argue that beyond the sensible password length or a 30-40 bytes, brute-forcing passwords is impractical for everyone unless they have billions of years to do it.
  • If the password hashing algo is bcrypt, we throw an error for inputs larger than 72 bytes. This is exactly what symfony/security component does, and it probably is the least surprising way, although we break BC for those who use quarter a tweet for their password.


Both approaches above will make the password hashes truly portable and make it easier to integrate WordPress with other platforms, be it importing users to WordPress, exporting WordPress users to another, or cross-authentication.

c) @mbijon suggested using batch processing and usermeta. This is not necessary, nor actually possible. The conversion process would need the original password input by the user, so it can only be done at the time they actually login, same as the old MD5 upgrade process. So, we modify the existing process to upgrade from old $P passwords to new ones in the exact same manner as it currently does for MD5 hashes.

You are right we will not be able to batch-process passwords, nor it would be a good idea to have millions of WordPress sites suddenly bcrypt millions of strings during an update.

d) Centralize the use of password_hash (and password_verify), so that filters can be applied to it across the board, in case somebody wants to change $algo or $options.

Someone also mentioned config.php directive too, noting that it is too much of a responsibility for a plugin to alter hashing algorithms mid-flight. But I agree that it should be neatly centralized and allow room for modifications.

e) For obvious reasons, the phpass library will need to remain to verify existing hashes before they are upgraded. No plans to remove or deprecate it are necessary. We still convert old MD5 hashes, after all.

Yep. It's a small-enough library and it is only loaded on-demand even at this point. It's a third party library, but it would be nicer to have hash_equals() in there instead of the current === comparator.

#6 @ayeshrajans
5 years ago

If we pick to pre-hash passwords, lets go with base64-encode sha2. Hash length shouldn't matter a lot because we will truncate it to 72 either way.

#7 in reply to: ↑ 5 @Otto42
5 years ago

Replying to ayeshrajans:

Thanks a lot for the in-depth comment @Otto42, very helpful.

In that plugin, one of the use cases was to make it possible for WordPress and a custom site to share login, so having raw passwords gone through password_hash helped to keep things cleaner, so it became a design choice to ignore the 72 char limit.

I see the points raised about bcrypt's 72 byte limitation, and while they are valid and can solve the very problem of "unlimited" length passwords, I would like to propose that we do not pre-hash passwords.

Hashes are the same regardless of where they are run. Truncating passwords at 72 doesn't make them any more portable.

hash( 'sha384', $string ) returns the same on any system for the same $string.

Truncating passwords to 72 bytes reduces security for no reason.

Also consider that we profess to support multibyte character sets, and 72/4 = 18, and 18 characters is certainly not long enough for what I would consider to be a secure password, even if it is in non-latin characters.

If we pick to pre-hash passwords, lets go with base64-encode sha2. Hash length shouldn't matter a lot because we will truncate it to 72 either way.

The "sha2" is not a method I know. The php hash function supports many things, and I'm suggesting we use "sha384". SHA384 produces 48 bytes, which is 64 in base64, which fits into our 72 limit.

#8 @bgermann
5 years ago

sha2 is a group name for sha256, sha384, sah512, ...

#9 @bgermann
5 years ago

To address the portability it would make sense to include the prehashing in the stuff that you can change via the filter. I.e. one of the filter's parameters should be the clear text password after the 4096 character limit check.

#10 @Otto42
5 years ago

I'm confused as to why you think using the raw password would be more portable? Can you explain this in more detail?

The hashed password is still the password. It doesn't change from being hashed in different systems.

#11 @bgermann
5 years ago

I do not mean operating system portability but cryptographic portability. You should not weaken the password strength for a hashing algorithm other than bcrypt by leaving out the prehashing.

#12 @bgermann
5 years ago

"portability" was a reference to "Both approaches above will make the password hashes truly portable and make it easier to integrate WordPress with other platforms"

#13 @Otto42
5 years ago

I do understand that prehashing the password is a bad-thing because of the reduction of entropy, and that the likelihood of somebody using a 72 byte password is slim. However, given 4 byte character encodings being likely going forward, then this is a real issue. Albeit perhaps not one to be addressed directly in this fashion.

Is there any indication that PHP is going to address the 72 byte issue going forward, in some kind of reasonable time frame? If there is, then using a straight password_hash() and implementing password_needs_rehash() in the login would be secure-enough to stick with PHP defaults.

I know that ARGON2 was considered at being the new default over bcrypt at some point, and it doesn't have a length limitation. But I have not seen any progress on if/when that will become the default algo used.

#14 @ayeshrajans
5 years ago

thank you @bgermann - I agree with the filters for password rules. I wonder if there is any filters already to validate an incoming password (such as a strength check for passwords when user enters a new password).

@Otto42 - the 72 byte limit is from bcrypt and not PHP itself. regardless of the caller, bcrypt will truncate I out to 72 bytes.

I also always typed 72 "bytes" because that's what matters for bcrypt. For a typical password that's typed on a US keyboard layout (ASCII and a few more Latin characters), the character count is the same as byte count. We will be using UTF-8, which means we will use the least amount of bytes per character (compared to UTF-16 and 32). The upper limit for password length would be between 72 ASCII characters or 18 complex emojis (such country flags, or thumbs up emojos with a specific color.

as for the portability, I understand the hashes are deterministic functions. It loses portability as in you cannot use the same password hashes between two different systems. for example, one might have a Laravel application that uses WordPress users table to authenticate users; If we were to impose any pre-processing for the password before it goes through password_hash(), all other systems need to do this now, thus, losing the portability. Ideally one would be able to throw in an ORM to WordPress users table and authenticate users.

Last edited 5 years ago by ayeshrajans (previous) (diff)

#15 @deadduck169
5 years ago

You definitely want to avoid truncating the hash at all costs. SHA2 algorithms are secure when the entire hash is included, whereas truncated hashes are not proven to be secure and could introduce a vulnerability.

I also second the motion to just let passwords be truncated at 72 bytes. Arguably you are not adding any additional security at the password at this point. There are currently 143859 Unicode characters defined (and growing) out of over 1.1 million possible characters. This means that there are currently about 1092 possible 18 character Unicode passwords (i.e. more than the total number of atoms in the known universe). I doubt the security bottleneck will be in your password at that point.

Last edited 5 years ago by deadduck169 (previous) (diff)

#16 @stgoos
4 years ago

I follow the original ticket for years now and with the minimum PHP requirement for WordPress now being at 5.6 it's about time to get this finally sorted in my opinion.

Btw - my 2 cents regarding your 2nd point:

2.Expose a filter for plugins
... We can expose a filter that WordPress core emits so plugins can change the hashing algorithm if necessary.

Is that desireable at all? Shouldn't this be controlled via a setting in wp-config.php which makes it clear that the WordPress installation will use an alternative hashing algorithm instead?
That way no one can be taken by surprise that the passwords have been changed by simply activating a plugin that for some reason feels the need to use an alternative hashing algorithm.

#17 @stgoos
2 years ago

Is there already any sign of this getting implemented in the next WordPress release, lets say 6.2?

The original ticket was opened over 10 (!!) years ago on June 20, 2012. It's kinda unreal that a security topic like this can't be tackled within 10 years...

This ticket was mentioned in Slack in #core by pikamander2. View the logs.


19 months ago

#19 @craigfrancis
14 months ago

  1. As to using a hash like sha256 before using bcrypt (ref 72 character limit, and null bytes), an approach used by DropBox and ParagonIE, we should be aware of Hash/Password Shucking (video).

    i.e. other websites do use fast hashes like sha256, and get compromised, so the attacker can take those those hashes and check them against these bcrypt hashes, if any work, then their password guessing can target the fast hash instead (ignoring/shucking the bcrypt part).

    But this is just one thing to consider, as many people use same or similar passwords on different websites, or simple/well-known passwords (e.g. RockYou), the attacker can try those apporaches instead... personally, I wouldn't worry about 73+ character passwords (the first bit is probably strong enough already), and I'd be tempted to remove null bytes so any characters afterwards would still be considered.
  1. PHP 8.4 is changing the default bcrypt cost from 10 to 12, this might be problematic for low powered servers, or shared hosting providers.

    Imagine getting ~20 logins at a time (a server hosting many websites, or maybe someone sends an email to a few thousand customers); with a cost of 10, that will slow down requests for every other non-login page, e.g. from ~0.09 seconds to ~1.1 seconds, while not good, it's still manageable; cost of 11 takes that to ~2.1 seconds; cost of 12 goes to ~4.2 seconds (timings from a simple ab -n 200 -c 20 to call password_hash, and while true; do curl -o /dev/null -s -w '%{time_total}\n' to time a basic page while this is running)... that said, a denial of service attack is a different consideration (i.e. don't allow hundreds of requests from a single source).
  1. Maybe we should also use normalizer_normalize($password, Normalizer::FORM_KD), ref UTF normalization, and 800-63B, section 5.1.1.2.
Last edited 14 months ago by craigfrancis (previous) (diff)

#20 @johnbillion
6 weeks ago

  • Milestone changed from Awaiting Review to 6.8
  • Owner set to johnbillion
  • Status changed from new to accepted

#21 @johnbillion
13 days ago

  • Milestone 6.8 deleted
  • Resolution set to duplicate
  • Status changed from accepted to closed

For the sake of consolidating the conversation, I'm going to close this ticket as a duplicate of #21022 where I'm proposing this change for 6.8. Thanks all.

Note: See TracTickets for help on using tickets.