WordPress.org

Make WordPress Core

Opened 22 months ago

Last modified 8 weeks 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 3.6-early has-patch
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 (2)

use_bcrypt.diff (1.8 KB) - added by harrym 17 months ago.
Patch to use bcrypt by default
21022.2.diff (2.8 KB) - added by iandunn 8 weeks ago.
Updates a few more calls to PasswordHash()

Download all attachments as: .zip

Change History (49)

comment:1 scribu22 months 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.

comment:2 scribu22 months ago

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

Last edited 22 months ago by scribu (previous) (diff)

comment:3 JustinSainton22 months ago

+1 for constant.

comment:4 th2322 months 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.

comment:5 toscho22 months ago

  • Cc info@… added

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

comment:6 scribu22 months 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.

comment:7 nacin22 months 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.

comment:8 jammycakes18 months 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 18 months ago by jammycakes (previous) (diff)

comment:9 harrym17 months 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.

comment:10 follow-up: Otto4217 months 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.

harrym17 months ago

Patch to use bcrypt by default

comment:11 harrym17 months ago

  • Keywords has-patch added

I added a patch against trunk.

comment:12 dd3217 months 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.

comment:13 Otto4217 months 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.

comment:14 follow-up: nacin17 months ago

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

comment:15 in reply to: ↑ 14 Otto4217 months 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.

comment:16 harrym17 months 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.

comment:17 in reply to: ↑ 10 ryanhellyer17 months 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 ;)

comment:18 follow-ups: westi17 months 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.

comment:19 in reply to: ↑ 18 ryanhellyer17 months 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 17 months ago by ryanhellyer (previous) (diff)

comment:20 in reply to: ↑ 18 ; follow-up: harrym17 months 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?

comment:21 in reply to: ↑ 20 ; follow-ups: ryanhellyer17 months 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 17 months ago by ryanhellyer (previous) (diff)

comment:22 in reply to: ↑ 21 ; follow-up: nacin17 months 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.

comment:23 in reply to: ↑ 21 harrym17 months 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.

comment:24 follow-up: rmccue17 months 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.

comment:25 in reply to: ↑ 24 westi17 months 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.

comment:26 follow-up: harrym17 months 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.

comment:27 in reply to: ↑ 26 ; follow-ups: ryanhellyer17 months 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 17 months ago by ryanhellyer (previous) (diff)

comment:28 tomdxw17 months ago

  • Cc tom@… added

comment:29 in reply to: ↑ 27 bpetty17 months 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.

comment:30 in reply to: ↑ 27 SergeyBiryukov17 months ago

Replying to ryanhellyer:

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

Related: #21737

comment:31 mbijon17 months ago

  • Cc mike@… added

comment:32 in reply to: ↑ 22 ; follow-up: ryansatterfield17 months 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 17 months ago by ryansatterfield (previous) (diff)

comment:33 rmccue17 months ago

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

Punting to 3.6-early for discussion then.

comment:34 in reply to: ↑ 32 ; follow-ups: Otto4217 months 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.

comment:35 in reply to: ↑ 34 ryansatterfield17 months ago

Do you know that a large majority of people who use WordPress barely know how to turn their cookies on? 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.

Version 0, edited 17 months ago by ryansatterfield (next)

comment:36 in reply to: ↑ 34 ryansatterfield17 months 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.

comment:37 follow-up: ryansatterfield17 months 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?

comment:38 in reply to: ↑ 37 SergeyBiryukov17 months 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/

comment:39 iandunn14 months ago

  • Cc ian_dunn@… added

comment:40 travisnorthcutt14 months ago

  • Cc travis@… added

comment:41 josephscott11 months ago

  • Cc joseph@… added

comment:42 freddyware11 months ago

  • Cc frederick.ding@… added

comment:43 betzster10 months ago

  • Cc j@… added

comment:44 koke4 months 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.

comment:45 koke4 months ago

  • Cc jorge@… added

comment:46 Otto428 weeks 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).

iandunn8 weeks ago

Updates a few more calls to PasswordHash()

comment:47 ocean908 weeks ago

  • Keywords has-patch added
Note: See TracTickets for help on using tickets.