WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 4 days ago

#21022 new enhancement

Allow bcrypt to be enabled via filter for pass hashing

Reported by: th23 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.4
Component: Security Keywords: 2nd-opinion has-patch 4.5-early
Focuses: Cc:

Description

Hi,

following recent discussions on password security and how to best prevent any hackers can leverage password table they might have got I looked into the phpass used for WordPress.

While I in principle understand why WordPress uses the compatibility mode of it, I would like to see some flexibility for those who don't need the compatibility.

Thus I would propose to change in wp-includes/pluggable.php all occurances of

$wp_hasher = new PasswordHash(8, true);

to

$wp_hasher = new PasswordHash(8, apply_filters('phpass_compatibility_mode', true));

This would allow users to easily change via plugin from the "not so secure" compatibility mode (only salted MD5) of phpass to a more secure setting (bcrypt) in case no compatibility with other applications is required.

The plugin changing the encryption methog could then as easy as

function phpass_bcrypt() {

return false;

}
add_filter('phpass_compatibility_mode', 'phpass_bcrypt');

Attachments (3)

use_bcrypt.diff (1.8 KB) - added by harrym 4 years ago.
Patch to use bcrypt by default
21022.2.diff (2.8 KB) - added by iandunn 3 years ago.
Updates a few more calls to PasswordHash()
21022.3.diff (19.6 KB) - added by tomdxw 4 days ago.
Use bcrypt where possible, upgrade portable hashes, show error message when downgrading to < PHP 5.3.7

Download all attachments as: .zip

Change History (83)

#1 @scribu
4 years ago

I'm wondering if this would be better as a wp-config.php constant, since it's not something that you would want random plugins to mess with.

#2 @scribu
4 years ago

There's also the fact that these functions are already pluggable, which means they can be replaced wholesale by a plugin.

Last edited 4 years ago by scribu (previous) (diff)

#3 @JustinSainton
4 years ago

+1 for constant.

#4 @th23
4 years ago

Support the constant as well.

Have seen the pluggability and actually went down that road for now, however I don't think it the best way to require replacing the whole 2 functions just for changing that setting.

#5 @toscho
4 years ago

  • Cc info@… added

What is the difference between a plugin defining a constant and a plugin using a filter?

#6 @scribu
4 years ago

The difference is that a second plugin can't redefine the constant. Also, it would be good if the constant was always set, either in wp-config.php by the user or before loading plugins, by WordPress.

#7 @nacin
4 years ago

As much as I hate pluggable functions, this is precisely what they're good for. I'd rather see a plugin override both methods than a filter or a constant. (Have you seen http://codex.wordpress.org/Editing_wp-config.php? Let's not add something to that page that could seriously screw up your install.)

Also, for what it's worth, a plugin can simply do $wp_hasher = new PasswordHash(8, false); before WordPress does. No need for plugging, or hooking, or defining.

#8 @jammycakes
4 years ago

IMO, bcrypt needs to be made the default, out of the box option on all systems that support it. The idea that WordPress admins should have to go hunting for a plugin or tweak configuration options to do this scares me, simply because most of them won't unless (a) they are well versed in web security, (b) they know that WordPress uses a weak alternative by default, and (c) they consider it to be an issue worth worrying about.

People often underestimate the seriousness of MD5 and the SHA-* algorithms being "less secure." They aren't just less secure: thanks to developments in password cracking in the past few years using GPU- and FPGA- based software, they are totally useless. Programs such as oclHashCat even have an option specifically to crack passwords in WordPress databases -- and the rate at which they can do so is terrifying. If you're not making a strong password hashing algorithm the default, out of the box option, you're exposing your users to unacceptable and unnecessary risk.

For what it's worth, you can do this without breaking backwards compatibility. It should be possible to include some code that can identify which algorithm you're using, and you can upgrade your users' passwords to the new option when they log in. You would also need to be able to do this if you wanted to increase the work factor passed into bcrypt every so often to allow for improvements in cracking technology.

(For reference, see the original "just use bcrypt" article: http://codahale.com/how-to-safely-store-a-password/)

Last edited 4 years ago by jammycakes (previous) (diff)

#9 @harrym
4 years ago

+1 for switching the default. I think someone moving from new PHP to old PHP and finding their site is:

  • unlikely
  • easily resolved by resetting your account password

And the normal upgrade path (where someone has lots of MD5 passwords and then starts using bcrypt) is a non-issue as PHPass will detect whatever algo was used and react appropriately.

I've just discovered this ticket having already written a plugin (!) that makes this change, if anyone wants to give it a go (https://github.com/dxw/wp_bcrypt/archive/master.zip).

I think this should just be changed in the core though.

#10 follow-up: @Otto42
4 years ago

PHPass actually checks for the existence of CRYPT_BLOWFISH and then CRYPT_EXT_DES support, and uses the best method available when the $portable_hashes argument is set to false, falling back to MD5 otherwise. Thus, there's no downside to simply always using false here.

Now that we're on PHP 5.3 for core, every PHP install should have CRYPT_BLOWFISH, but even if it's strangely compiled without it, PHPass will continue to work.

My vote is for simply removing the "true" from the $portable_hashes argument altogether. We don't need it, it's more secure without it, and honestly it doesn't even need to be a configurable option.

@harrym
4 years ago

Patch to use bcrypt by default

#11 @harrym
4 years ago

  • Keywords has-patch added

I added a patch against trunk.

#12 @dd32
4 years ago

Now that we're on PHP 5.3 for core, every PHP install should have CRYPT_BLOWFISH, but even if it's strangely compiled without it, PHPass will continue to work.

We still require PHP 5.2.4.

#13 @Otto42
4 years ago

Fair enough, but regardless, PHPass is backward compatible on it's own. There's no downside to letting it use non-portable hashes. Worst case is you move a site from one server to another that doesn't have bcrypt and you have to reset your passwords.

#14 follow-up: @nacin
4 years ago

Enforcing portable hashes means that they are portable. A change like this could make deployments more difficult.

#15 in reply to: ↑ 14 @Otto42
4 years ago

Replying to nacin:

Enforcing portable hashes means that they are portable. A change like this could make deployments more difficult.

If you're deploying between systems running largely different versions of PHP, yes. Pretty thin case, seems to me.

#16 follow-up: @harrym
4 years ago

Yes. I agree with Otto.

Switching to bcrypt as the default will only affect a very small number of people (if any) who go from a newer version of PHP to an older one.

In any other case, PHPass figures out what to do.

Does anyone know how you can check for PHP feature X in PHP version Y? Can't find old versions of the manual anywhere and the current version says crypt has been supported since PHP 4, and CRYPT_BLOWFISH since 5.3, but doesn't say when CRYPT_EXT_DES was introduced.

#17 in reply to: ↑ 10 @ryanhellyer
4 years ago

This seems like something best switched over once PHP 5.3 is the required version. Otherwise there is a risk that someone needs to move a site from a server running one version of PHP supported by WordPress, but on moving to another server with a version of PHP supported then it may break due to the password hashing algorithm being missing.

PHPPass doesn't seem to be inherently insecure in itself, so there is no urgent need to change. Moving eventually is obviously a good idea, but I'm not convinced that now is the appropriate time.

Immediate solution ... bump the PHP requirement to 5.3 ;)

#18 follow-ups: @westi
4 years ago

I think we should do this, and I think we should make the password re-encrypting code upgrade to a bcrypted password on login like we do for md5.

#19 in reply to: ↑ 18 @ryanhellyer
4 years ago

Replying to westi:

I think we should do this, and I think we should make the password re-encrypting code upgrade to a bcrypted password on login like we do for md5.

Dang. I hadn't thought of forcing the upgrade like that. That would solve most migration problems since most people would automatically receive a new password hash seamlessly anyway.

EDIT: I think I had too much tequila before posting this. Re-encrypting makes the problem worse, not better. Your idea is still a good one, as it forces the newer/better encryption to be used, just not for the reasons I suggested when I posted this comment.

Last edited 4 years ago by ryanhellyer (previous) (diff)

#20 in reply to: ↑ 18 ; follow-up: @harrym
4 years ago

Replying to ryanhellyer:

Otherwise there is a risk that someone needs to move a site from a server running one version of PHP supported by WordPress, but on moving to another server with a version of PHP supported then it may break due to the password hashing algorithm being missing.

Surely this risk is minute? You'd have to move from a server running 5.3.x to one running 5.2.x. And it's trivially solvable by changing your password.

What's involved in increasing the requirement from 5.2 to 5.3? That feels non-trivial.

Replying to westi:

I think we should do this, and I think we should make the password re-encrypting code upgrade to a bcrypted password on login like we do for md5.

That's exactly what the plugin does (linked above) although I didn't include that in the patch. Happy to resubmit if it's looking likely to be accepted?

By "this" did you mean wait for 5.3 or change it now?

#21 in reply to: ↑ 20 ; follow-ups: @ryanhellyer
4 years ago

Replying to harrym:

Surely this risk is minute? You'd have to move from a server running 5.3.x to one running 5.2.x. And it's trivially solvable by changing your password.

I run a few sites with many thousands of logged in users. Forcing them to all upgrade their passwords at once would be quite problematic. It's fairly unlikely we'd ever downgrade the server during a move like that of course, but you never know what sort of weird things people might do without realising the ramifications until afterwards.

What's involved in increasing the requirement from 5.2 to 5.3? That feels non-trivial.

That's more of a political question I guess. Changing it is physically trivial, but judging by how hard it was to move from PHP 4 to 5.2 I'm guessing having the requirements change will not be so trivial.

Last edited 4 years ago by ryanhellyer (previous) (diff)

#22 in reply to: ↑ 21 ; follow-up: @nacin
4 years ago

Replying to harrym:

What's involved in increasing the requirement from 5.2 to 5.3? That feels non-trivial.

Only 31% of WordPress installs run 5.3. I don't see this happening before 2014.

#23 in reply to: ↑ 21 @harrym
4 years ago

  • Keywords has-patch removed

Replying to ryanhellyer:

I run a few sites with many thousands of logged in users. Forcing them to all upgrade their passwords at once would be quite problematic.

That seems fair enough.

Replying to nacin:

Replying to harrym:

What's involved in increasing the requirement from 5.2 to 5.3? That feels non-trivial.

Only 31% of WordPress installs run 5.3. I don't see this happening before 2014.

Wow. I'm surprised it's that low.

So it sounds like switching the default is not likely to happen soon. Given that:

  • It's going to be a while before the default can be changed
  • A third of installs could immediately benefit

Can we reconsider making a define to control portability?

Happy to resubmit a patch if it's a goer, including the hash upgrade on login.

#24 follow-up: @rmccue
4 years ago

  • Keywords 2nd-opinion punt added; dev-feedback removed

duck_ and I discussed this on IRC, especially with regard to the current cost factor of 8 in PHPass. On my local box, hashing 1k passwords takes ~2s, giving ~2ms per password. This should probably be bumped up slightly. The upcoming password hashing API in PHP 5.5 uses 10 as the cost, and we might want to switch to using that too. There's a compatibility library.

Note that the compat library requires PHP 5.3.7+, since "PHP prior to 5.3.7 contains a security issue with its BCRYPT implementation"; duck_ noted the relevant crypt_blowfish update and the bug description:

The majority of hashes (but not all of them) for passwords containing characters with the 8th bit set are incompatible with OpenBSD's (really nasty, but no security impact here).

What's worse, approximately 3 in 16 passwords containing a single character with the 8th bit set have 1 to 3 characters immediately preceding the 8-bit character ignored. With more than one character with the 8th bit set, things may be even worse.

Thus, those passwords may be much easier to crack than expected.

i.e. With non-ASCII characters, there are severe security issues.

Due to the above bug, I don't think we should be switching to bcrypt on by default until we require PHP 5.3.7+, which is not for a while. I'm going to recommend punting for now, but I'd like a second opinion before I do.


Replying to jammycakes:

People often underestimate the seriousness of MD5 and the SHA-* algorithms being "less secure." They aren't just less secure: thanks to developments in password cracking in the past few years using GPU- and FPGA- based software, they are totally useless. Programs such as oclHashCat even have an option specifically to crack passwords in WordPress databases -- and the rate at which they can do so is terrifying. If you're not making a strong password hashing algorithm the default, out of the box option, you're exposing your users to unacceptable and unnecessary risk.

If that's the case, I think we should increase the cost factor ASAP (which is backwards compatible, since it's stored in the salt IIRC).

Replying to harrym:

Wow. I'm surprised it's that low.

Check http://wordpress.org/about/stats/ for slightly more detail.

#25 in reply to: ↑ 24 @westi
4 years ago

Replying to rmccue:

duck_ and I discussed this on IRC, especially with regard to the current cost factor of 8 in PHPass. On my local box, hashing 1k passwords takes ~2s, giving ~2ms per password. This should probably be bumped up slightly. The upcoming password hashing API in PHP 5.5 uses 10 as the cost, and we might want to switch to using that too. There's a compatibility library.

Note that the compat library requires PHP 5.3.7+, since "PHP prior to 5.3.7 contains a security issue with its BCRYPT implementation"; duck_ noted the relevant crypt_blowfish update and the bug description:

The majority of hashes (but not all of them) for passwords containing characters with the 8th bit set are incompatible with OpenBSD's (really nasty, but no security impact here).

What's worse, approximately 3 in 16 passwords containing a single character with the 8th bit set have 1 to 3 characters immediately preceding the 8-bit character ignored. With more than one character with the 8th bit set, things may be even worse.

Thus, those passwords may be much easier to crack than expected.

i.e. With non-ASCII characters, there are severe security issues.

Due to the above bug, I don't think we should be switching to bcrypt on by default until we require PHP 5.3.7+, which is not for a while. I'm going to recommend punting for now, but I'd like a second opinion before I do.


Replying to jammycakes:

People often underestimate the seriousness of MD5 and the SHA-* algorithms being "less secure." They aren't just less secure: thanks to developments in password cracking in the past few years using GPU- and FPGA- based software, they are totally useless. Programs such as oclHashCat even have an option specifically to crack passwords in WordPress databases -- and the rate at which they can do so is terrifying. If you're not making a strong password hashing algorithm the default, out of the box option, you're exposing your users to unacceptable and unnecessary risk.

If that's the case, I think we should increase the cost factor ASAP (which is backwards compatible, since it's stored in the salt IIRC).

Replying to harrym:

Wow. I'm surprised it's that low.

Check http://wordpress.org/about/stats/ for slightly more detail.

I think it would be nice to see if we can modify WP's behaviour to use bcrypt where we can soon, but probably not for 3.5.

We should probably target this investigation for 3.6-early with a view to backporting into the 3.5 branch as well.

For now I think that pluggable function replacement is better plugin implementation than a filter for enabling bcrypt on sites where it is safe to do so.

#26 follow-up: @harrym
4 years ago

  • Keywords dev-feedback added

+1 for increasing the cost factor. Jammycakes is absolutely right, and the factor is indeed stored along with the salt. There are no compatibility issues involved there.

We do regular password audits for our clients - breaking WordPress password hashes isn't even slightly difficult. We can try >20k candidates per second, and that's on a fairly pedestrian office machine. Custom rigs with multiple GPUs can do hundreds of thousands of attempts per second. This is not a minor problem!

Can we have dev-feedback on making the use of portable hashes a configurable option, via a define?

I see no reason why people shouldn't be able to customise this behaviour in wp-config if they're aware of the tradeoffs.

#27 in reply to: ↑ 26 ; follow-ups: @ryanhellyer
4 years ago

Even at 100 billion attempts per second, it would still take over 1000 years to crack a password with only 12 characters in it, and that's assuming only numbers and English characters. So 100,000 attempts per second doesn't seem like anything worth worrying about.

The situation in which that could be a problem, is when users use horrendously insecure passwords. Moving to a more secure hash will unfortunately not stop users from choosing a password of 123abc which would still be trivial to crack, even with bCrypt. So perhaps an alternative solution to this is to implement a minimum password strength system like the following plugin?
http://www.itsananderson.com/plugins/minimum-password-strength/

I have seen multiple sites "hacked" due to insecure passwords. Passwords like "password", "letmein" and "admin" appear to be scarily common. Since implementing that plugin, I haven't see any examples of this occurring thankfully. Implementing it seems like it would get to the core of the problem a little more directly and effectively than changing the hashing algorithm.

Last edited 4 years ago by ryanhellyer (previous) (diff)

#28 @tomdxw
4 years ago

  • Cc tom@… added

#29 in reply to: ↑ 27 @bpetty
4 years ago

Replying to ryanhellyer:

The situation in which that could be a problem, is when users use horrendously insecure passwords. Moving to a more secure hash will unfortunately not stop users from choosing a password of 123abc which would still be trivial to crack, even with bCrypt. So perhaps an alternative solution to this is to implement a minimum password strength system like the following plugin?
http://www.itsananderson.com/plugins/minimum-password-strength/

I have seen multiple sites "hacked" due to insecure passwords. Passwords like "password", "letmein" and "admin" appear to be scarily common. Since implementing that plugin, I haven't see any examples of this occurring thankfully. Implementing it seems like it would get to the core of the problem a little more directly and effectively than changing the hashing algorithm.

Ryan has a very good point here. The fact that WP already uses per-password-salts and stretching with a well respected password hashing library is actually pretty good regardless of what hashing method is used. There's no point in bumping server resource requirements, and extending page response times for registration and login past 2 seconds (not even including everything beyond the hash) when the actual problem that needs to be solved is password strength.

#30 in reply to: ↑ 27 @SergeyBiryukov
4 years ago

Replying to ryanhellyer:

So perhaps an alternative solution to this is to implement a minimum password strength system

Related: #21737

#31 @mbijon
4 years ago

  • Cc mike@… added

#32 in reply to: ↑ 22 ; follow-up: @ryansatterfield
4 years ago

While I really care about security, it isn't logical to switch the supported version to 5.3. Why? Well, 3,383,560 servers are currently running 5.2. Only 3,475,453 servers support PHP 5.3. If WordPress stopped supporting 5.2 there would be an outrage. The problem stems from PHP not putting in native support for more secure hash types before 5.3. I agree with Nacin on the fact that we should use plugins until at least 2014. If you even know about password hashing, then finding a plugin won't be hard. If you want to double check my findings on the PHP versions go to shodanhq.com and do some searches.

Replying to nacin:

Replying to harrym:

What's involved in increasing the requirement from 5.2 to 5.3? That feels non-trivial.

Only 31% of WordPress installs run 5.3. I don't see this happening before 2014.

Last edited 4 years ago by ryansatterfield (previous) (diff)

#33 @rmccue
4 years ago

  • Keywords 3.6-early added; 2nd-opinion punt dev-feedback removed

Punting to 3.6-early for discussion then.

#34 in reply to: ↑ 32 ; follow-ups: @Otto42
4 years ago

Replying to ryansatterfield:

While I really care about security, it isn't logical to use PHPass and switch the supported version to 5.3.

As I stated above, PHPass is *backwards compatible* with 5.2 just fine, even in non-portable password mode.

#35 in reply to: ↑ 34 @ryansatterfield
4 years ago

Do you know that a large majority of people who use WordPress barely know how to turn their computers on or delete their cookies? I do WordPress tutoring through my company and sometimes just to help people out. The "logic" I hear is quite a wake up call to how we should program to help them. Unless PHPass can detect that It has been moved to a server that supports PHP 5.2 instead of PHP 5.3 and then automatically changes the password encryption, then it isn't logical to have it as a WordPress default.

I don't work for WordPress, but I know the end-user mind and so does the WordPress team. I know what you are saying is easy for us geeks, but it isn't for a large portion of WordPress clients. People use WordPress because it is the easiest CMS available.

This will have to wait until at least 2014, although I expect a lot of servers to still support PHP 5.2 in 2014.

Replying to Otto42:

Replying to ryansatterfield:

While I really care about security, it isn't logical to use PHPass and switch the supported version to 5.3.

As I stated above, PHPass is *backwards compatible* with 5.2 just fine, even in non-portable password mode.

Last edited 4 years ago by ryansatterfield (previous) (diff)

#36 in reply to: ↑ 34 @ryansatterfield
4 years ago

After further research I agree with Otto. PHPass will automatically change the hash in non-portable mode and can detect the PHP version on it's own. Maybe this could get into 3.5 or 3.5.1? I don't see a PHPass submitted patch. I will try to submit a patch soon. I am busy today and part of Friday. I have to do Holiday stuff.

Otto, I am sorry for my initial response. I replied that way since I've worked with people who spent a week on their own without my help to learn the most simplest thing about WordPress and still couldn't figure it out. I was wrong to automatically assume the user would have to tell WordPress that they had changed PHP versions. I hope everyone has a good Thanksgiving.

Replying to Otto42:

Replying to ryansatterfield:

While I really care about security, it isn't logical to use PHPass and switch the supported version to 5.3.

As I stated above, PHPass is *backwards compatible* with 5.2 just fine, even in non-portable password mode.

#37 follow-up: @ryansatterfield
4 years ago

  • Keywords 3.5 2nd-opinion added; 3.6-early removed

I have a patch so it will use MD5 if it is on 5.2, but if it is on 5.3 or higher it will use sha-512. I am setting up subversion right now since git tortoise isn't working great. If PHPass documentation is right, the existing users hash should be automatically switched if the environment changes. The only issue I didn't fix in the patch was altering the table, since varchar only holds 64 characters and sha-512 is 128.

This should be able to get out in 3.5, shouldn't it?

#38 in reply to: ↑ 37 @SergeyBiryukov
4 years ago

  • Keywords 3.6-early added; 3.5 removed

Replying to ryansatterfield:

This should be able to get out in 3.5, shouldn't it?

No, we are long past the enhancement stage for 3.5: http://make.wordpress.org/core/version-3-5-project-schedule/

#39 @iandunn
4 years ago

  • Cc ian_dunn@… added

#40 @travisnorthcutt
4 years ago

  • Cc travis@… added

#41 @josephscott
3 years ago

  • Cc joseph@… added

#42 @freddyware
3 years ago

  • Cc frederick.ding@… added

#43 @betzster
3 years ago

  • Cc j@… added

#44 @koke
3 years ago

About the required PHP version, PHP has other hashes available since 5.1.2, like SHA1/SHA256/SHA512. See hash_algos() and hash_hmac.

#45 @koke
3 years ago

  • Cc jorge@… added

#46 @Otto42
3 years ago

Relevant information: http://nakedsecurity.sophos.com/2014/02/16/syrian-electronic-army-hacks-forbes-spills-1000000-user-records/

TL;DR: The Forbes.com site (running WordPress) was breached and about a million rows of the users table dumped. The password hashes were exposed.

Though the hashing is still okay using the iterated MD5, it seems that the bar should probably be raised. If not switching to better algorithms when they are available, then the iteration count should be raised.

Personally, I would prefer using better algorithms, which means, at the very least, we should implement a filter or define to allow people to disable the forced use of portable hashes (aka, forcing MD5).

@iandunn
3 years ago

Updates a few more calls to PasswordHash()

#47 @ocean90
3 years ago

  • Keywords has-patch added

#48 @tomdxw
13 months ago

Is this a change that could make it into 4.4?

#49 @mark8barnes
12 months ago

It seems wrong that in 2015 we're still not using bcrypt for password hashing, at least for systems that support it. I understand why portability is a good thing, but not if it makes the majority of systems vulnerable.

  • The chances of people downgrading from PHP 5.3+ to 5.2 are diminishing by the day.
  • Downgrading is least likely to happen on a large site with lots of users, which is where there is the biggest potential problem.
  • It would be trivial create an alert that would display if the admin attempted to log in when passwords were bcrypted but the server didn't support bcrypt. That way if someone does move from 5.3 to 5.2, they'd very soon understand the problem and be able to reverse the change.

#50 @Otto42
12 months ago

Note that the "use case" for leaving portable hashes enabled no longer makes any real sense, since WordPress is already not portable in other ways. WordPress now behaves differently on different versions of MySQL, using utf8mb4 on certain versions but not others.

#51 follow-up: @mojorob
12 months ago

This may be a daft question, however I suspect WordPress already checks version of PHP somewhere. Therefore is it not possible to have a check if PHP is => 5.5.0 then use the native password hashing functions? (password_hash etc.)

It would seem a fairly good & light option for those hosting on more recent versions of PHP.

#52 in reply to: ↑ 51 ; follow-ups: @mark8barnes
12 months ago

Replying to mojorob:

Therefore is it not possible to have a check if PHP is => 5.5.0 then use the native password hashing functions? (password_hash etc.)

That's not the worry. The worry is that if this is enabled for PHP 5.5+, then someone downgrades from PHP 5.5 to PHP 5.3, then bcrypt will no longer work, and people won't be able to log-in without resetting their passwords.

#53 in reply to: ↑ 52 ; follow-up: @mojorob
12 months ago

Replying to mark8barnes:

Replying to mojorob:

Therefore is it not possible to have a check if PHP is => 5.5.0 then use the native password hashing functions? (password_hash etc.)

That's not the worry. The worry is that if this is enabled for PHP 5.5+, then someone downgrades from PHP 5.5 to PHP 5.3, then bcrypt will no longer work, and people won't be able to log-in without resetting their passwords.

It is that bad for a person to click "forgot password" and have a link emailed to them to create a new password?

#54 in reply to: ↑ 53 ; follow-up: @chriscct7
12 months ago

  • Keywords 3.6-early removed

Replying to mojorob:

Replying to mark8barnes:

Replying to mojorob:

Therefore is it not possible to have a check if PHP is => 5.5.0 then use the native password hashing functions? (password_hash etc.)

That's not the worry. The worry is that if this is enabled for PHP 5.5+, then someone downgrades from PHP 5.5 to PHP 5.3, then bcrypt will no longer work, and people won't be able to log-in without resetting their passwords.

It is that bad for a person to click "forgot password" and have a link emailed to them to create a new password?

Yes, because a majority of the users will know they were entering the previously correct password and won't understand they need to reset their passwords. Also on a larger install, with hundreds of thousands of users, particularly if the site deals with eCommerce, this could provide for a massive headache in terms of support.

#55 in reply to: ↑ 54 @mojorob
12 months ago

Replying to chriscct7:

Replying to mojorob:

Replying to mark8barnes:

Replying to mojorob:

Therefore is it not possible to have a check if PHP is => 5.5.0 then use the native password hashing functions? (password_hash etc.)

That's not the worry. The worry is that if this is enabled for PHP 5.5+, then someone downgrades from PHP 5.5 to PHP 5.3, then bcrypt will no longer work, and people won't be able to log-in without resetting their passwords.

It is that bad for a person to click "forgot password" and have a link emailed to them to create a new password?

Yes, because a majority of the users will know they were entering the previously correct password and won't understand they need to reset their passwords. Also on a larger install, with hundreds of thousands of users, particularly if the site deals with eCommerce, this could provide for a massive headache in terms of support.

Then we're back to a point earlier made by someone else that "it would be trivial to create an alert that would display if the admin attempted to log in when passwords were bcrypted but the server didn't support bcrypt." In that case the request link form can be shown for everyone when login fails the first time (not just admin), and additionally an email sent to admin to alert them. A simple message saying something like "we have changed our login system and require everyone to reset their passwords" could help too.

However, for the kind of sites you mention I would have thought the admins would know what they're doing, and the user case is quite small.

#56 @knutsp
12 months ago

I think mojorob has a valid point here. The really big sites will or should know what they are doing, and I can't imagine them suddenly downgrading to PHP 5.3 without knowing what they will be facing in this case.

You can already do much more bad/unwise things already, with hooks, filters, dropins and pluggables.

I'm all for responsible thinking when adding hooks, like not inviting to do hazardous and harmful things, but this is meant to enable enhanced security, not?

#57 in reply to: ↑ 52 @toscho
12 months ago

Replying to mark8barnes:

That's not the worry. The worry is that if this is enabled for PHP 5.5+, then someone downgrades from PHP 5.5 to PHP 5.3, then bcrypt will no longer work, and people won't be able to log-in without resetting their passwords.

One very exotic and unlikely use case cannot be a reason to lower the security for everyone.

#58 @Otto42
12 months ago

I'd be careful about "unlikely". There was a fair amount of complaints about the utf8mb4 changes, from people running different versions of mysql on test/demo sites vs. production ones.

In that case, they built sites locally, then transferred them with mysql dumps. This would cause the same problem for local sites running 5.5 and production running 5.2.

Still, my view is to simply reset the passwords in such a case, so I'm still for this change. Maybe making detection of the case is possible too, like was done with the md5 password change originally.

Last edited 12 months ago by Otto42 (previous) (diff)

#59 @mojorob
12 months ago

I also think it's a good idea from now on that for WordPress on PHP5.5+ it should completely bypass the version of PHPass bundled and use the native function. That will ensure the latest algorithm is used as per PHP version on the server.

#60 @mikeschroder
12 months ago

Despite the user complications here -- if they switch PHP versions or between hosts, for instance -- I'm very much behind this change.

Let's make passwords more secure for users. :)

#61 @Otto42
12 months ago

Using the native 5.5 password_hash functions is a noble idea, but one for a separate ticket given the compat issues.

Let's stick on point for this ticket. All that is needed is to remove the true parameter from where it exists. Maybe some detection code, for the extreme minority of users that will be moving between hosts or systems where this will cause an issue.

Last edited 12 months ago by Otto42 (previous) (diff)

#62 @tomdxw
12 months ago

One thing that I haven't seen mentioned in this ticket is silently upgrading password hashes. This bit of code updates a user's password hash when they log in if they're still using the compatible hashing scheme:

add_filter('check_password', 'my_check_password', 10, 4);
function my_check_password($check='', $password='', $hash='', $user_id='') {
    if($check && substr($hash, 0, 3) == '$P$') {
        wp_set_password($password, $user_id);
    }

    return $check;
}

Is it worth including some functionality like that in this ticket? If not, then password hashes will only get updated when users reset their passwords, which means that even after admins upgrade to the latest version of WP it could still be years in the future when the majority of their users are using bcrypt.

Also, note that when the password hashing algorithm is bcrypt, the password reset process breaks. See ticket #33904.

#63 in reply to: ↑ 52 @deadduck169
11 months ago

Replying to mark8barnes:

That's not the worry. The worry is that if this is enabled for PHP 5.5+, then someone downgrades from PHP 5.5 to PHP 5.3, then bcrypt will no longer work, and people won't be able to log-in without resetting their passwords.

Actually, according to here, it's versions < 5.3.7 and also that haven't had the $2y fix backported into them that are incompatible. Of the servers using PHP 5.3, most will likely be using either >= 5.3.7 or a version with $2y backported into it.

The stats show that at the time of writing only 11.2% of Wordpress servers currently use PHP 5.2. The chance of someone moving from a PHP 5.3.7+ server back to a 5.2 server are probably pretty negligible, especially since 5.2 has been past its end of support since the beginning of 2011, and if changing hosts and given the choice, most people would choose a server with more recent software, not older software.

I feel that while not using bcrypt by default we are throwing the baby out with the bath water. We can easily detect whether the user's PHP is compatible with $2y (see the check function here), so even if there's a 0.1% chance that someone might migrate from a compatible version to an incompatible version, all we need to do is display a message to users after they have attempted to log in, like this:

  1. User enters their login info
  2. Server retrieves password hash from the database
  3. Server sees that the hash uses $2y and PHP is <= 5.3.7.
  4. Server checks for $2y compatibility using the function linked above
  5. If incompatible, display the following message to the user:

    Warning: This installation of Wordpress was migrated from a new version of PHP to an older one. Unfortunately we are unable to verify your password, so please [reset it]. This only needs to be done once.

So it's a minor inconvenience for the few odd installs that for some reason migrate to an old and unsupported PHP version, but increased security for the ~90% of installs that are currently using PHP >= 5.3.7 (and increasing every day). I think that's a fair trade-off.

Last edited 11 months ago by deadduck169 (previous) (diff)

#64 @tomdxw
10 months ago

  • Keywords 4.5-early added

Can we have bcrypt in 4.5?

#65 in reply to: ↑ 16 @nacin
9 months ago

Replying to harrym:

Switching to bcrypt as the default will only affect a very small number of people (if any) who go from a newer version of PHP to an older one.

This remains my three-year-old concern here. I believe this is a substantially less of a concern of mine now, given that our 5.2 numbers have continued to erode and are under 10% at this time. It's always been unlikely for anyone to deliberately deploy downward to 5.2; but it's now also increasingly unlikely odds for anyone to unwittingly to move to a different host and end up on 5.2.

HOWEVER, we need to handle this with good UX. What does that mean? If you try to log in on a site and the password hash for the account you are trying to log into is $2a$, and your site does not support bcrypt, then we need to give them an error message and take them to a very, very good documentation page on wordpress.org.

Additionally, PHP < 5.3.7 has a vulnerability in the Blowfish implementation. We will need to decide if we should skip < 5.3.7, rather than just < 5.2, when turning on Blowfish. That will likely be the cleanest way to do this, based on my rusty understanding of CVE-2011-2483. For more, start with http://www.openwall.com/lists/announce/2011/06/21/1 and http://php.net/security/crypt_blowfish.php.

Finally, I'd like to see what kind of stats we have on .org that can help us understand how often a site downgrades the PHP version. I'm asking @dd32 for help there.

#66 follow-up: @Otto42
9 months ago

Actually, looking at this one again, I think it's been so long on this one that we should instead consider switching to the PHP 5.5+ password_hash() function, and including a compatibility library such as https://github.com/ircmaxell/password_compat/ for older PHP versions.

Note that that library is PHP 5.3.7+ only as well, for the same bcrypt security reasons, so we still have issues there. We could consider including both PHPass and the password_hash compat library, and then using whichever makes sense for the current PHP version.

The user_pass field was expanded to 255 characters in #33904 for exactly this reason, BTW.

Essentially, if we're going to support stronger password encryption methods, then we're ripping the bandaid off anyway. Let's go ahead and modernize to use the built in methods when possible. If we're have to have PHP version checks and such anyway, then we might as well do it up right.

#67 in reply to: ↑ 66 @mojorob
9 months ago

Replying to Otto42:

Actually, looking at this one again, I think it's been so long on this one that we should instead consider switching to the PHP 5.5+ password_hash() function, and including a compatibility library such as https://github.com/ircmaxell/password_compat/ for older PHP versions.

I suggested the (PHP5.5+) native password_hash 3 months ago, and I still think it's the way to go. So I would agree with such a switch.

All except one of the WordPress sites I look after are now running on PHP7, and still using the wp-bcrypt plugin due to what some might suggest is an excessive need to retain backward compatibility. Surely when it comes to password security a better approach is to keep up with standards for those who can. For those who can't/won't then include an alternative as suggested. As mentioned before when there is a downgrade of PHP on a live site, then it could be made to have minimal impact - any large sites would (should?) know of potential issues when downgrading PHP.

Rather than simply having bcrypt in WP4.5, I'd suggest moving over to native password_hash in a manner suggested by Otto.

#68 @dd32
9 months ago

I'd like to see what kind of stats we have on .org that can help us understand how often a site downgrades the PHP version.

Unfortunately our stats for this are not very good, it appears that there's probably a lot of sites which are switching between versions of PHP very often which is making our stats very noisy - I'm going to assume these are test or dev sites.. (I'm working on filtering these out so hopefully I'll have some better data to share in a week or so)

I don't think a user would intentionally switch to another host which runs PHP 5.2, however someone who maintains WordPress sites might move a site onto their infrastructure, and run into that problem. I don't see this being an issue to that segment of users though.

Using password_hash() in 5.5+ could be a better idea than switching to bcrypt with phpass directly, however, with only ~35% of 4.3/4.4 sites running PHP 5.5/5.6/7 the user experience of a PHP downgrade (no matter how rare) would need to be far better than simply using phpass+bcrypt in PHP 5.3.7+. The number of hosts which are still PHP 5.4 is common enough that a user may switch to one.

I do however wonder if the ideal user experience would simply be a login error with a link to WordPress.org & a password reset email, is something such as Whoops! PHP can no longer decrypt your password, <a href="w.org">find out why</a> or <a>reset your password</a> as user friendly as the rest of WordPress which just works?

Last edited 9 months ago by dd32 (previous) (diff)

#69 @Otto42
9 months ago

I agree that this is a UX thing, and giving solid reasoning and information to the user about why is important.

If the password-hash is detectable (via length or header), then this is possible. 3 months ago, we didn't have PHP 7 getting adopted, or 4.4 with increased length password fields. This seems doable now, but maybe making the password hash mechanism more generic and pluggable is the way forward?

#70 @Otto42
9 months ago

Also consider the efforts involved in the two factor plan, insofar as to ensure email resets are working.

#71 @nacin
9 months ago

Replying to @Otto42:

If the password-hash is detectable

The password algorithm is stored as the first few characters. $P$ are portable hashes, $H$ are phpBB portable hashes, $2a$ is bcrypt, $2y$ is bcrypt generated by crypt() >= PHP 5.3.7.

My comment didn't suggest using phpass over password_hash(). Yes, we should probably look at password_hash() so we don't need to worry about phpass's internal salt generation. (That said, the author of phpass is also the author of crypt_blowfish.)

Replying to @dd32:

Using password_hash() in 5.5+ could be a better idea than switching to bcrypt with phpass directly, however, with only ~35% of 4.3/4.4 sites running PHP 5.5/5.6/7 the user experience of a PHP downgrade (no matter how rare) would need to be far better than simply using phpass+bcrypt in PHP 5.3.7+. The number of hosts which are still PHP 5.4 is common enough that a user may switch to one.

It's actually possible we could do 5.5 + password_hash( $algo = PASSWORD_BCRYPT ) and still be portable down to 5.3.7 without changing anything, because phpass simply uses crypt() internally so we'd be able to evaluate a $2y$ hash. So I'm not actually sure we'd need to use password_compat.

I'd be mostly interested in figuring out where exactly the new stuff should go. If we do it in our pluggable functions, we'll need to check if we're using portable hashes ($P$, $H$) there so we can send it to phpass (and then upgrade it), otherwise we'll need to put this into phpass. Sounds like we may need our own way to encapsulate (and to get this logic out of pluggable functions).

#72 @mattheweppelsheimer
8 months ago

I don't think a user would intentionally switch to another host which runs PHP 5.2, however someone who maintains WordPress sites might move a site onto their infrastructure, and run into that problem. I don't see this being an issue to that segment of users though.

Agreed, but I just want to point out that "intentionally" is a key word. Over the years we've had a few clients move away from our management to cut costs, then call us in a panic when their cheapskate new host's older PHP version breaks things. Anecdotal but this makes me think it's small sites like these, run by people clueless about PHP versions, who are most likely to get bit.

However we implement better hashing, +1 to @dd32's suggestion (or something similar):

"Whoops! PHP can no longer decrypt your password, <a href="w.org">find out why</a> or <a>reset your password</a>`

Last edited 8 months ago by mattheweppelsheimer (previous) (diff)

#73 @wturrell
8 months ago

(Hello, I am new.)

As the ticket is nearly four years old, would the fastest way to make a little progress, but with minimal disruption and whilst keeping our future options open, be implementing the original constant idea, so $portable_hashes can be false?

Even if the decision is not to activate bcrypt by default for new installations, it would at least allow informed users to increase their security level right now. As already mentioned, phpass stores the algorithm in the initial characters, so it's not computationally expensive to determine which type of password it is (i.e. you don't have to try each in turn) and I can't see how it would restrict future choice of encryption.

Also, I note there's multiple copies of this conditional in core, could it be refactored for DRY purposes?

        if ( empty( $wp_hasher ) ) {
                require_once ABSPATH . WPINC . '/class-phpass.php';
                $wp_hasher = new PasswordHash( 8, true );
        }

#74 @DeveloperWil
7 months ago

I like @dd32's idea of simply adding an email and password reset for passwords that cannot be decrypted.

You could even pre-empt issues by storing the current PHP version in the DB during the update check and trigger at the very least an admin email when the version of PHP has been changed, or especially so, downgraded.

That would at least give site owners information on why a site has "broke" as in @mattheweppelsheimer's examples.

Storing the PHP version in the DB could also enable secure password hashing on new installations if PHP >= 5.5

Considering how insecure MD5 is and how many sites are powered by WordPress can this issue get some traction?

Last edited 7 months ago by DeveloperWil (previous) (diff)

#76 @ryan
7 months ago

  • Switch to password_hash() and password_verify(). This is very straightforward code. https://github.com/roots/wp-password-bcrypt/blob/master/wp-password-bcrypt.php
  • If log in fails and password_hash() does not exist and the password hash is not portable, send a password reset email and show a notice on the log in screen explaining that password reset is required and an email has been sent. This will cover the scenario of someone deploying a db with password_hash() hashes on PHP < 5.3.7.
  • Publish a support doc and link to it from the notice.

Should we pull in the back compat versions of password_hash() and password_verify() for php >= 5.3.7 and < 5.5 for the scenario of moving forward from phpass to password_hash()? In other words, do we want to use back compat versions to move sites forward or use them only for the edge case of deploying a database with password_hash() hashes on an old version of php. Not using them to move forward would mean sites with PHP < 5.5 would stay on phpass, even if they are >= 5.3.7 and can use the back compat funcs.

#77 @ryan
7 months ago

I pinged Solar Designer, author of phpass, for thoughts on migration. Thread:

https://twitter.com/rboren/status/706890076135882755

This ticket was mentioned in Slack in #meta by iandunn. View the logs.


6 months ago

#79 @tomdxw
5 days ago

I looked over the past year of comments on this ticket and made a TODO list:

  1. add password_hash()/password_verify() functions from this library: https://github.com/ircmaxell/password_compat/
  2. if PHP version >= 5.3.7, use the PHP password_hash()/password_verify() functions (for lower versions of PHP, keep using PasswordHash class from phpass)
  3. when a user logs in, if the site is using bcrypt and their password is hashed using portable hashes, update their hash to a bcrypt hash
  4. when a user logs in, if the site is *not* using bcrypt and their password is hashed using bcrypt (i.e. when PHP is downgraded), show a message saying "Sorry, something has gone wrong and you must reset your password. <a href="https://codex.wordpress.org/Foobar">More information</a>."

Does this look correct? Have I missed anything? If somebody produced a patch containing the above changes, could we get it committed?

Edit: Removed comment about automatically sending a password reset because the code that sends a password reset lives in wp-login.php so it's not easily accessible from other places.

Last edited 4 days ago by tomdxw (previous) (diff)

@tomdxw
4 days ago

Use bcrypt where possible, upgrade portable hashes, show error message when downgrading to < PHP 5.3.7

#80 @tomdxw
4 days ago

Attached a patch.

  • I've left the password-protected posts feature alone
  • But every other place where the PasswordHash class was being used, that's been replaced with calls to a new class (the new class still uses HashPassword() and CheckPassword() methods so most password-handling code is unchanged)
  • The new class checks whether the PHP installation supports the password_hash/password_verify functions (and loads a compatibility library for PHP >= 5.3.7 and < 5.5.0)
  • It falls back to using the PasswordHash class for PHP < 5.3.7
  • I added two filters: one handles upgrading password hashes automatically, and the other provides an explanation when a user logs in and WP is unable to use the password hash found in the database

Of course it needs a bit of polish before it's ready to be committed, but is this the right approach?

Note: See TracTickets for help on using tickets.