Opened 12 years ago
Last modified 36 hours ago
#21022 accepted enhancement
Use bcrypt for password hashing; updating old hashes
Reported by: | th23 | Owned by: | johnbillion |
---|---|---|---|
Milestone: | 6.8 | Priority: | normal |
Severity: | normal | Version: | 3.4 |
Component: | Security | Keywords: | has-patch needs-testing has-unit-tests |
Focuses: | Cc: |
Description (last modified by )
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 (4)
Change History (191)
#2
@
12 years ago
There's also the fact that these functions are already pluggable, which means they can be replaced wholesale by a plugin.
#4
@
12 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
@
12 years ago
- Cc info@… added
What is the difference between a plugin defining a constant and a plugin using a filter?
#6
@
12 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
@
12 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
@
12 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/)
#9
@
12 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:
↓ 17
@
12 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.
#12
@
12 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
@
12 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:
↓ 15
@
12 years ago
Enforcing portable hashes means that they are portable. A change like this could make deployments more difficult.
#15
in reply to:
↑ 14
@
12 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:
↓ 65
@
12 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
@
12 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:
↓ 19
↓ 20
@
12 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
@
12 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.
#20
in reply to:
↑ 18
;
follow-up:
↓ 21
@
12 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:
↓ 22
↓ 23
@
12 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.
#22
in reply to:
↑ 21
;
follow-up:
↓ 32
@
12 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
@
12 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:
↓ 25
@
12 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
@
12 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:
↓ 27
@
12 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:
↓ 29
↓ 30
@
12 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.
#29
in reply to:
↑ 27
@
12 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
@
12 years ago
Replying to ryanhellyer:
So perhaps an alternative solution to this is to implement a minimum password strength system
Related: #21737
#32
in reply to:
↑ 22
;
follow-up:
↓ 34
@
12 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.
#33
@
12 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:
↓ 35
↓ 36
@
12 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
@
12 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.
#36
in reply to:
↑ 34
@
12 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:
↓ 38
@
12 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
@
12 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/
#44
@
11 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.
#46
@
11 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).
#49
@
9 years 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
@
9 years 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:
↓ 52
@
9 years 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:
↓ 53
↓ 57
↓ 63
@
9 years 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:
↓ 54
@
9 years 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:
↓ 55
@
9 years 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
@
9 years 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
@
9 years 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
@
9 years 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
@
9 years 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.
#59
@
9 years 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
@
9 years 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
@
9 years 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.
#62
@
9 years 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
@
9 years 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:
- User enters their login info
- Server retrieves password hash from the database
- Server sees that the hash uses $2y and PHP is <= 5.3.7.
- Server checks for $2y compatibility using the function linked above
- 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.
#65
in reply to:
↑ 16
;
follow-up:
↓ 114
@
9 years 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:
↓ 67
@
9 years 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
@
9 years 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
@
9 years 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?
#69
@
9 years 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
@
9 years ago
Also consider the efforts involved in the two factor plan, insofar as to ensure email resets are working.
#71
@
9 years 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
@
9 years 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>`
#73
@
9 years 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
@
9 years 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?
#76
@
9 years 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.
This ticket was mentioned in Slack in #meta by iandunn. View the logs.
9 years ago
#79
@
8 years ago
I looked over the past year of comments on this ticket and made a TODO list:
- add password_hash()/password_verify() functions from this library: https://github.com/ircmaxell/password_compat/
- 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)
- 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
- 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.
@
8 years ago
Use bcrypt where possible, upgrade portable hashes, show error message when downgrading to < PHP 5.3.7
#80
@
8 years 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?
#84
@
8 years ago
Would be great to get bcrypt support merged into 4.8. Let me know if there's anything I can do to improve the patch.
This ticket was mentioned in Slack in #core-passwords by tomdxw. View the logs.
7 years ago
#87
@
7 years ago
honestly I would also agree to add native PHP's Password function and give somewhere in the admin panel even an option to set things like the cost or the algorithm (with PHP7.2 for example we will be getting argon2 as an option)
also regarding users with WAY too old PHP versions that are EOL since half an eternity (5.2 was ended in January two thousand ELEVEN), are there stats on how the PHP version split is for people that use the latest (or second latest) version of WP? I wouldnt be expecting too many people who are on older versions to update anyway)
for hosters that only support versions that have been EOL for over 2 years, those hosters should in my opinion be sued for intentionally risking the security of anyone involved.
I would say that when you guys plan to do 5.0 it would be time to throw some things out which are really in the way of security.
Over 25% of Wordpress installations according to stats are running 4.5 or lower, meaning they havent updated for almost a year and a half, in comparison, less than 15% run php 5.2 and 5.3, both are versions EOL for over 3 years at the time of posting.
and for downgrading, another person already mentioned fun with different database versions, so that would be another problem, where PHP cant even do ANYTHING
#88
follow-up:
↓ 89
@
7 years ago
Use PHPs password_hash, seriously. How is phpass and md5 still used in 2017??
#89
in reply to:
↑ 88
@
7 years ago
Replying to tobivreal:
Use PHPs password_hash, seriously. How is phpass and md5 still used in 2017??
Because WordPress won't support as a minimum PHP 5.5 (when password_hash became available) until 2025 or so... possibly 2026, we'll see.
#90
@
7 years ago
well it would be enough to ax down PHP 5.2 and anything <5.3.7 and use the compatibility library, also even 5.3 as a whole was axed in august 2014, over 3 years ago and 5.2 is 6 years and 9 months EOL.
and even 5.5 is EOL for over a year already. I think we can drop 5.3 and go with 5.4 at least even though that is also EOL since over 2 years already. a hoster who doesnt even have that should go out of business anyway.
and then again, at least for anyone on PHP>=7 you wouldnt expect those to downgrade lower than 5.5 so it would be at least safe enough to enable proper hashes at least for those, or just go with the best you can and if there's a downgrade you just say password reset and finished
#91
@
7 years ago
MITRE have assigned the identifier CVE-2012-6707 to refer to the use of a weak MD5-based hash to store passwords in WordPress.
By the way there's a patch which I think should satisfy all the concerns raised in this ticket: https://core.trac.wordpress.org/ticket/21022#comment:80
#92
@
7 years ago
@tomdxw I fully agree to this. in case of a downgrade you can just forget the password and set a new one (and you shouldnt use a "personal" password for an installation that has ugly hashes anyway.
but that this now is an official CVE is nice to see.
but a question I posted still stands.
How many of the super-outdated PHP versions are on a recent wordpress installation? are there stats for that?
because only those are in the scope of this anyway.
and even then on a later version of WP like 5.0 PHP <5.3.7 could be axed. also since the older versions apparently still get patched for quite a while, I mean even 3.7 still got an update in September and 3.7 as a whole is almost 4 years old.
PHP 5.2 is as already said already EOL for over 6 years an 9 months and if 4.9 which is sceduled to be on nov 14 would be the last 4.x release and contained MD5 hashes, and it would also get about 4 years of patches PHP5.2 would be EOL since 10 years and 10 months from that hypothetical End of WP4.9.
so while it would be sad to see MD5 in 4.9 it would make sense to enter at least a transition with what @tomdxw said and in 5.0 ax it completely.
while I am not a fan of axing down old versions for mundane reasons, this is a pretty important security thing and really slowing down WP in this aspect.
but when we add password_hash, I would love to have a setting about the parameters (using argon2 in PHP7.2, or just changing the cost depending on what you like)
#93
@
7 years ago
This ticket was opened 5 years ago. There was a patch added 13 months ago without any feedback or response.
@tomdxw has done a great job of summarizing the issues and pushing this forward with a patch. He's even asked twice for feedback about his approach/solution.
After a certain point you have to ask yourself how much the WordPress core team cares about this issue?
#94
@
7 years ago
@swalkinshaw full ack on what you say.
but when it is so desperately needed to use some portable hash, why not use an at least marginally safer hash as a base like sha256 or sha512 and iterate on that.
Sure this isnt a very good Idea, but it certainly is better than MD5 and should fullfill the compatibility down to PHP5.2 as I went into the museum of the PHP releases grabbed PHP 5.2.0, started up a shell and let it list hash_algos which had SHA512.
but the keyword stays on desperately. the approach of either
1) just using whats available and when someone really is either stupid or unlucky enough to downgrade to 5.2, they should reset the password
2) axing off PHP<5.3.7 completely in the next major and going full on the password_hash
should really be done rather than upgrading the utterly crappy portable password hash into a less junk but still pretty bad but portable password hash.
#95
@
7 years ago
- Keywords 5.0-early added; 4.9-early removed
so 4.9 has release a while ago and of course nothing happened.
version 5.0 is scheduled for 2018, and adding a new full version means that we also could break some things and considering we get many years of security updates even for very old versions, 3.7 even recieved the update on the end of october marking a support time for greater than 4 years, even though it is used just about 0.3% which isnt bad, and I would think if 4 gets similarly lengthy security updates then I think there's no problem to go around breaking stuff on the next major version.
also we still have almost 25% running around on 4.5 or less meaning they are still on a minor from 1 and a half years ago, and they havent bothered updating to a new one. the stats dont show the patchlevel but unless auto updates have been enabled on those, chances are they never recieved any update ever
also small other thing. once 5.0 releases (march 2018 in the earliest considering they run about 4 months per update, though chances are it's more because it's a major) even PHP 5.6 will have less than a year to live and the same for 7.0. At the end of this month PHP 7.2 will release and WP5 should really embrace it by for example allowing the user to use argon2 for passwords.
I honestly dont think it's good in the long run to have a CVE lying around in wordpress for (from the point of WP5.0 release) almost or even more than 6 years, this can seriously affect the reputation at least in the set of people who know IT stuff at least a bit.
and while the following idea is crazy and I know it, if we REALLY want to keep way too old PHP versions alive we should at least switch from MD5 to SHA-1 or if old PHP can work with it, a SHA2 algorithm, and while SHA1 has been Shattered already, it IS less than half as fast as MD5. on a set of 8 GTX1080 MD5 gets over a massive 200 Billion hashes per second. a single overcloecked 1080TI puts in almost 34 Billion hashes, so using 8 of these gets almost 272 Billion hashes. an 8 character completely random password (of all the 94 print-ascii chars) gets us about 6 HOURS in the worst case, considering that most passwords arent random chances are they are broken rather quickly. and while a 10 char password may still take around 6 years on this setup,speeds are only increasing and we are just on GPUs. imagine throwing ASICs into the mix, this is getting crazy and something really needs to be done.
by the way I axed the 4.9-early tag because 4.9 has been released and that tag wont help anyone anymore.
#96
@
7 years ago
I've been following this thread for five years. My original comment stands. Once support for for PHP 5.2 is dropped, the password hashing system should change.
I think this debate is better held in a ticket specific to the PHP version number, as I don't see how this ticket can progress without the minimum version number being increased. Once the minimum version number is increased, implementing a better hashing algorithm seems like an obvious and much needed upgrade. In the mean time, we just have to tolerate reduced security.
#97
@
7 years ago
but we could at least switch from MD5 to SHA, which isnt a lot better, but hey, it IS better
This ticket was mentioned in Slack in #core by dingo_d. View the logs.
7 years ago
#101
follow-up:
↓ 102
@
6 years ago
7 years. Nothing in WP5. You can't delegate this to plugins or user modifications as the average user isn't even aware of this issue.
I cannot justify any new WP installs for clients until this issue is resolved.
#102
in reply to:
↑ 101
@
6 years ago
Replying to tobivreal:
7 years. Nothing in WP5. You can't delegate this to plugins or user modifications as the average user isn't even aware of this issue.
I cannot justify any new WP installs for clients until this issue is resolved.
Exactly my thoughts, would be nice to see this *finally* being resolved in the very near WP5.0.x future.
#103
@
6 years ago
As WordPress 5.2 is finally going to switch minimum PHP version to 5.6 this ticket should be revisited. With this change, password_hash can be used without any backwards compatibility layer.
#104
follow-up:
↓ 105
@
6 years ago
It is worth noting that switching to password_hash would effectively limit password lengths to 72 bytes (the bcrypt algorithm ignores the rest, so PHP truncates the password to that length).
This should be discussed, as the last time we limited password lengths, we limited it to 4096 bytes.
I'm for switching to password_hash, BTW. Just thought this should be known.
#105
in reply to:
↑ 104
;
follow-up:
↓ 106
@
6 years ago
Replying to Otto42:
It is worth noting that switching to password_hash would effectively limit password lengths to 72 bytes (the bcrypt algorithm ignores the rest, so PHP truncates the password to that length).
If your passwords are 72 bytes long, I can guarantee your security bottleneck is not in the password length limitation.
#106
in reply to:
↑ 105
@
6 years ago
Replying to deadduck169:
If your passwords are 72 bytes long, I can guarantee your security bottleneck is not in the password length limitation.
I don't disagree, but it's worth noting in case any patch tries to keep the existing 4096 limit.
#107
follow-up:
↓ 110
@
6 years ago
I kept the class-phpass.php file for the rehashing, however it would be fine to incorporate the used parts inline. How should we handle this?
I did not change the 4096 character limit unit test yet so this one fails. Should I just remove the test or introduce an explicit check for 72 characters in the authentication code and the test?
#108
@
6 years ago
- Keywords 5.0-early removed
- Severity changed from normal to major
- Summary changed from Allow bcrypt to be enabled via filter for pass hashing to Use bcrypt for password hashing; updating old hashes
This ticket was mentioned in Slack in #core by bgermann. View the logs.
6 years ago
#110
in reply to:
↑ 107
@
6 years ago
Replying to bgermann:
I did not change the 4096 character limit unit test yet so this one fails. Should I just remove the test or introduce an explicit check for 72 characters in the authentication code and the test?
I'm not a programmer, but Dropbox has probably a nice approach to this problem - using SHA512 along with the bcrypt.
Maybe this information will help somehow someone.
Dorpbox's related article
#111
@
5 years ago
I'm not a programmer, but Dropbox has probably a nice approach to this problem - using SHA512 along with the bcrypt.
The best strategy here, in our opinion, would be to base64_encode(hash('', '', true))
instead of hash('', '')
. You'll get a higher information density before the 72 character truncation.
Even unmodified bcrypt is a lot more secure than the current state of affairs with MD5. Bcrypt-SHA2 is the most robust strategy until the whole world can move on to Argon2.
This ticket was mentioned in Slack in #core by timothybjacobs. View the logs.
5 years ago
#113
@
5 years ago
@paragoninitiativeenterprises depending on what characters are safe to use in bcrypt one might even be able to use Base91 instead of base64
http://base91.sourceforge.net/
which basically does the same as base64 but with more characters to choose from thereby raising the information density and thereby allowing more stuff into the password before stuff gets truncated.
or obviously IF bcrypt is binary safe one wouldnt need to encode at all and 64 bytes fits into the 72 limit of bcrypt no problem.
#114
in reply to:
↑ 65
@
5 years ago
Regarding Base64/basE91...
The truncation of passwords to 72-chars happens automatically in password_hash()
. Using baseXY methods does not increase the supported password length and does not increase the keyspace.
As for increasing info density, I'm not sure of the point in this situation @paragoninitiativeenterprises. Using base64_encode( password_hash('', '' true) )
could be un-done with a one-time operation on a leaked DB. then you'd be right back to rainbow table attacks.
<red herring alert...>
@paragoninitiativeenterprises:
Did you mean to suggest password_hash( base64_encode( $password ) )
?
I think that's not really the right debate for a thread aiming to upgrade from MD5 ... but it also runs against the recommendations in NIST 800-63-3:
Pre-encoding does appear to offer some marginal amount of improvement on bad passwords... First, by converting "abc123" -> "YWJjMTIz". But password complexity is largely ineffective against modern rainbow tables. Longer passwords (ideally passphrases) are a strong recommendation instead of complexity.
Base64 does offer a 4/3rds stretching effect on passwords. Unfortunately, the stretching effect of base64 means that password_hash's built-in truncation will limit passwords to 54 characters. Anything above that length would be discarded. This means a reduced keyspace and decreases overall or network-effect security. For example:
- A 54-char input will base74 to 72-chars. Then
password_hash
would use the whole 72-chars - A 55-char input would base64 to 74-chars and
password_hash
would truncate that to 72-chars
A better option (once password_hash() is in-use of course) would be to have a "sliding salt" that ensured a minimum-length of 72-chars, ie: password_hash( $password . $salt_72_char, PASSWORD_DEFAULT )
. Even a short password would get salted to max-out the 72-char limit in password_hash.
This strategy would allow password_verify()
to STILL WORK, and also ensure the larger 14-16 char MD5 rainbow tables do nothing if your DB ever is leaked.
#115
@
5 years ago
My earlier comment is meant to cut off a potential red herring to this long-running ticket. But it could also be seen as a pretty big red herring itself.
To bring things back to the point: Let's get rid of MD5 hashing.
I'm in favor of implementing bog standard PHP 5.6 password_hash( $password, PASSWORD_BCRYPT )
.
I think if work here focuses on a good backbone of UX and hash-detection, then we'll have an easy path to best-case PASSWORD_DEFAULT
and Argon2 support in the future.
Next release:
- Add the code from
21022.4.diff
but don't removepasswordHash
yet - Implement hash-type detection for
$P$B
,$2y$
and$argon2i$
(maybe$2a$
?) - Using that hash-type detection: 1. Add unit tests for
$2y$
functionality. 2. DeprecatepasswordHash
and start testing forE_DEPRECATED
- Plus, plugins like "wp-password-bcrypt" will easily be able to test for support & capabilities.
- Add an Admin alert to eligible RCs and maybe one major-version of WordPress that detects if
$P$B
hashes are in-use. While PHP 5.6+ ensures support for BCrypt it would be good to warn of a pending password-expiration & length-limit. - Implement password-expiration for all
$P$B
hashes in Upgrade. Include an Action so Admins can opt to have this upgrade process send password-expiration emails or similar.- With hash detection this will ensure NOT expiring
$2y$
or other hashes (in case a site already has "wp-password-bcrypt" or similar installed).
- With hash detection this will ensure NOT expiring
- UX update to registration & pw-reset to inform users of the 72-char password length-limit.
Future release:
- REVISE the
password_hash( *, PASSWORD_BCRYPT)
code topassword_hash( *, PASSWORD_DEFAULT )
and add a test for$argon2i$
- Remove
passwordHash
#116
@
5 years ago
@mbijon
you kinda overlooked something, so let me summarize (still very long so take your time, there are both code examples and math ahead).
Bcrypt has a 72 length password limit
how to deal with longer Passwords, just truncating kinda sux, especially when we are playing with wordlist passwords?
You said yourself that passphrases are a strong recommendation, the big problem is the bcrypt password length limit though, the idea of wordlist passwords us completely right but the entropy per byte of the phrase is very low. if we knew the list, we could obciously convert this into a byte sequence and be done with it, but we don't.
The idea of @mobby2561 was to SHA-512 the password and throw THAT into bcrypt, basically
<?php $prehash=hash("sha512",$password); $final_hash=password_hash($prehash,PASSWORD_BCRYPT);
Paragon later meant that the hex output is kinda large with 128 chars and suggested to use a base64 hash instead of a hex one, like this:
<?php $prehash_bin=hash("sha512",$password,true); $prehash_enc=base64_encode($prehash_bin); $final_hash=password_hash(%prehash_enc,PASSWORD_BCRYPT);
now the prehash is down to 88 characters, respectable, but we can do better, was what I thought, and brought base91 to the table.
using a little character counting website SHA512 went for 79 characters, so basically just 7 characters get thrown out.
<?php include("base91.php"); $prehash_bin=hash("sha512",$password,true); $prehash_enc=base91_encode($prehash_bin); $final_hash=password_hash(%prehash_enc,PASSWORD_BCRYPT);
so the idea is basically to press as much of a password into the bcrypt as we can. obviously a password with more than 512 bits of entropy won't fit into SHA-512 without losing some entropy this way but it would fall out anyway with normal bcrypt.
Time for some Math:
the idea is that if we say go for a more optimal case and assume we have a really big wordlist (the one I extract from 1password usually has around 18k words, the one I got a few weeks ago is 18 319, it is 131 879 bytes long, subtracting 18318 (newlines) from it we get 113 561. dividing that by 18 319 gives us an average of 6,2 characters per word.
I think I am pretty generous here, diceware only comes along with 7776 words, so yeah you will probably need a LOT more characters to get the same amount of entropy.
so now we need some harder math so take a breath, look at this and let me explain every step.
https://www.wolframalpha.com/input/?i=log18319(2^512)*6.2
from the inside to outside I do the following.
first I let it calculate the possibilites for a 512-bit secure password, aka before we break off the entropy from SHA512.
second I use the log with a base of 18319 to calculate how many words are needed for that
(to clarify a log with base (say 2) of a number (say 8) = x solves the question
2x = 8 with x being obviously 3)
that is about 36 words, quite a lot and most people wont use that sure.
finally we multiply by 6,2 (the average word length and get (not including any spaces) 224 characters before we reach an entropy of 512 bits. no way that's gonna fit into bcrypt.
on the other hand This equation:
https://www.wolframalpha.com/input/?i=log2(18319^(72%2F6.2))
tells us how much we CAN fit into an average 72 character wordlist password based on the assumptions. around 164 bit, which is definitely VERY good for a password, but reminder that WP currently has had a 4096 character limit for passwords. a straight cut to 72 would be a but dramatic
so after the main math is out let's talk a bit more about the cutting.
the hex sha512 cut 128 to 72, we are down to 288 bit, base64 went down from 88 characters, 418,9 bit, I think we can see where this is going, and finally base91 with 79 chars has 466,6 bit after the 72 character truncation
#117
@
5 years ago
regarding the red herring and all the other I am gonna make an extra comment just that it is easier to read.
Let's be honest:
<?php password_hash(base64($password),PASSWORD_BCRYPT)
is one of the dumbest ideas ever aside from a little bit of rainbow table prevention, because it limits our options even further, as we actually artificially inflate the password.
the base64 character output is (ignoring padding) is 4/3 of the input. so basically flipping this upside down, the input is 3/4 of the output, meaning we would be down to 54 characters before base64, so let's just not do that.
about using password_DEFAULT instead of forcing bcrypt, while I am generally in big favor of argon there are 2 problems with that.
1) PHP spec says that password default can apply already 1 version after an algorithm has been introduced, meaning that a release without that new hash can be still on the supported list as security only (they generally support 3 at a time, with the last one being security only)
2) Argon2 shows the VERY real problem that just because you have a PHP version you aren't guaranteed to have a certain hash function
3) the idea of downgrading PHP on migration or whatever seems very real here in WP, after all this is what holds/held back this for SEVEN YEARS already.
so currently sadly bcrypt is the only really available hash function, even though I am not happy with it myself.
#118
@
5 years ago
I think you guys are way over-complicating things here:
1) Always use complete cryptographically secure hashing algorithms. These have been tried and tested for years, whereas their truncated forms have not. Truncated hashes are therefore insecure and using them means you're essentially rolling your own crypto.
2) Bcrypt allows up to 72 bytes (576 bits) of input. SHA-512 produces a 64-bit (512 byte) hash, which fits perfectly in Bcrypt, with space left over. There's absolutely no reason you can't input binary:
<?php $password = 'test'; $sha512_hash = hash('sha512', $password); $binary = hex2bin($sha512_hash); $password_hash = password_hash($binary, PASSWORD_DEFAULT); var_dump(password_verify($binary, $password_hash)); // Output: bool(true)
#119
follow-up:
↓ 121
@
5 years ago
There's absolutely no reason you can't input binary:
What do you think the output of the following code would be?
<?php $correctPW = '34a124424f065ae13936064ab366d9'; $bad = 'be6759bc425ed7b26c177cf53af82b1ed519'; $hash = password_hash(hash('sha512', $correctPW, true), PASSWORD_BCRYPT); var_dump( password_verify( hash('sha512', $bad, true), $hash ) );
I'll give you a hint: https://paragonie.com/blog/2016/02/how-safely-store-password-in-2016#bcrypt
Code output: https://3v4l.org/2FTaS
#120
@
5 years ago
now the prehash is down to 88 characters, respectable, but we can do better, was what I thought, and brought base91 to the table.
There's a subtle reason why we recommended Base64: Migrating from base-256 (raw bytes) to any base-N (where N is a power of 2) is easy to implement using constant-time arithmetic. For example: https://github.com/paragonie/constant_time_encoding/blob/55af0dc01992b4d0da7f6372e2eac097bbbaffdb/src/Base64.php#L213-L270
When your new base is not an even power of 2, you have to deal with non-constant-time operations. This isn't straightforward: Even multiplying two integers can leak timing information on some processors. https://www.bearssl.org/ctmul.html
RFC 4648 defines several binary-safe encodings that we can use: Base16 (Hex), Base32, Base32Hex, Base64, and Base64Url.
However, we don't actually need binary-safe here, what we need is simply "no NUL characters" (without introducing timing leaks would be preferred).
If we really wanted to squeeze as much of the SHA512 hash into the 72 character real estate as possible, we could define an alternative "Base128" where all of the output characters have the high bit set (i.e. they occupy the range \x80 - \xFF
; the complement to ASCII) and use that instead of Base64. This gives us a binary blow-up of 8/7 which comes out to about 74 characters and is guaranteed to never have a NUL byte. (For the purposes of this discussion, since the high bit is set, let's call this hypothetical "Noble-128" so it doesn't get mistaken for an RFC 4648 approved standard.)
And such an algorithm would be easy to implement: Take 7 bits of input, prefix it with 1 bit, that's your output. (You can even use the standard =
padding character for uneven inputs, since it's not a valid member of the \x80-\xFF
keyspace.)
Is this worth the extra engineering effort? Well, the keyspace of a base64-encoded SHA512 hash is roughly 432 bits.
You won't ever need more than 256. https://pthree.org/2016/06/19/the-physics-of-brute-force
In my opinion, bcrypt-sha512-base64 is the most elegant solution available to WordPress with a PHP 5.6 minimum version. Mostly because it's a one-liner and doesn't require a custom encoding scheme be created.
We can bikeshed this further, but a successful argument against base64 here means more technical debt and adding code that mere mortals do not understand in the WordPress codebase.
Of course, if that's worth squeezing an extra 72 bits out of something that's already 174 bits in excessive of a generous security boundary, I'm happy to hear you out.
#121
in reply to:
↑ 119
;
follow-up:
↓ 122
@
5 years ago
Funny @paragoninitiativeenterprises, I just found the pointer dereference in PHP's bcrypt: https://github.com/php/php-src/blob/master/ext/standard/crypt_blowfish.c#L613. Combining crypto methods is never a good idea, eh.
Wish you mentioned the NUL
dereference in your 1st post here. Good info though: https://paragonie.com/blog/2015/04/secure-authentication-php-with-long-term-persistence
For anyone else following ... ABSOLUTELY DON'T USE... password_hash($raw_sha256|384|512, PASSWORD_BCYPT);
.
The vulnerability is a two-step: (1) Raw SHA hashes allow null-bytes as valid characters. Then (2) the PHP implementation of bcrypt truncates the password-input at the 1st null byte (likely ALL C-based implementations of bcrypt do this).
Or, as @paragoninitiativeenterprises noted in the link just above, there's a 1-in-256 chance that the 1st character in a SHA hash is a NUL-byte (0.39% chance). Extrapolating, that means there's a 5.9% chance a 15+ character (edit: changed 15-char to 15+) password gets truncated early if you feed a raw SHA directly to password_hash()
.
So ... do NOT password_hash( hash( 'SHA_any', $password ) )
.
In light of that, I think there are two options:
password_hash(base64(sha_abc($password)), SOMETHING)
password_hash($password, SOMETHING)
Before anyone has any knee-spasms again: Note that bcrypt()
is a PBKDF-type cryto. These are sometimes called slow hashes. On the other side, all the SHA's are hash-generators or fast hashes.
In attacker terms, once they get past "easy" rainbow/shortening/lengthening/collision vulnerabilities they're left with brute force. On a GPU bcrypt's default setup/lengthening/salting yields <1,000 attempts/second. On the same GPU, SHA hashing yields ~1,000,000,000 attempts/second.
I can't help but worry your bcrypt-sha512-base64 solution will make jumping to PASSWORD-DEFAULT
harder @paragoninitiativeenterprises. But heck! it's still 106 better than SHA, and way closer to vanilla password_hash()
than we have now.
#122
in reply to:
↑ 121
@
5 years ago
Replying to mbijon:
Funny @paragoninitiativeenterprises, I just found the pointer dereference in PHP's bcrypt: https://github.com/php/php-src/blob/master/ext/standard/crypt_blowfish.c#L613. Combining crypto methods is never a good idea, eh.
Most cryptography vulnerabilities exist in the mortar, not the bricks.
I can't help but worry your bcrypt-sha512-base64 solution will make jumping to
PASSWORD-DEFAULT
harder @paragoninitiativeenterprises. But heck! it's still 106 better than SHA, and way closer to vanillapassword_hash()
than we have now.
It won't make it significantly difficult.
We just need to ensure a reasonable migration path exists for post-bcrypt password hashes, in the same spirit as https://paragonie.com/blog/2016/02/how-safely-store-password-in-2016#legacy-hashes and https://make.wordpress.org/core/2019/05/17/security-in-5-2
Back on focus: This ticket was originally targeting WordPress 3.4 when a PHP 5.2 minimum was unlike to change.
Surely with a minimum PHP of 5.6 (the story today) we can seriously consider migrating to bcrypt in a near-future release?
#123
@
5 years ago
Yeah @paragoninitiativeenterprises, that's what I was hoping to do with my 1st post today. I do think there are some non-tech steps to take, but using your bcrypt-sha-base64 solution eliminates the need for a password-length warning.
I was leaning toward a forced password-update over backward-compat hashes. It's a big security improvement over having copies of low-security md5's around for years.
But if anyone wants to weigh-in on PM stuff instead of crypto ... How about...?
- Add Paragon's bcrypt-sha-base64 solution and KEEP
passwordHash
- Implement hash-type detection for
$P$B
,$2y$
and$argon2i$
(maybe$2a$
?) - For the upgrade, add an
is_legacy_password = true
tousermeta.meta_key
for every user with a$P$B
hash. - Also during upgrade, using the hash-type detection:
- Add unit tests for
$2y$
functionality. - Batch update every
$P$B
hash into a$2y$
format. - Helps plugins like "wp-password-bcrypt" because the hash detection ensures NOT expiring existing
$2y$
or better hashes.
- Add unit tests for
- Hook the login Action to check for
is_legacy_password == true
and if so (1) Hash their submitted password withpasswordHash
andpassword_hash()
to login, (2) Prompt the user to change their password (including UX with a "why"). On new password creation, setis_legacy_password = false
(edited to note two-step hashing for 1st login when is_legacy_password == true
)
#126
follow-ups:
↓ 127
↓ 128
@
4 years ago
@SergeyBiryukov I think we can ax phpass altogether in new versions now that WP is committed to use recent PHP versions, and in fact that oldest version stated to work is something in 5.6
https://wordpress.org/about/requirements/
"WordPress also works with PHP 5.6.20"
or was WP downgrading ever a thing?
#127
in reply to:
↑ 126
@
4 years ago
Replying to my1xt:
@SergeyBiryukov I think we can ax phpass altogether in new versions now that WP is committed to use recent PHP versions, and in fact that oldest version stated to work is something in 5.6
https://wordpress.org/about/requirements/
"WordPress also works with PHP 5.6.20"
or was WP downgrading ever a thing?
+1 ;)
#128
in reply to:
↑ 126
@
2 years ago
Replying to my1xt:
@SergeyBiryukov I think we can ax phpass altogether in new versions now that WP is committed to use recent PHP versions, and in fact that oldest version stated to work is something in 5.6
https://wordpress.org/about/requirements/
"WordPress also works with PHP 5.6.20"
or was WP downgrading ever a thing?
Only when you have a plugin that isn't compatible yet with the newer version of WordPress, but that should only be a very temporarily situation. That said, for the more serious web admins this will only happen on their development/test/staging site and not (as in: never) on their production site.
But in all seriousness. It's plain ridiculous that this particular security topic/ticket (opened at June 20, 2012!!) has still not made it into the core of WordPress. The minimum PHP requirement for WordPress has gone up to PHP 7.4 a while ago already so that can't be the reason (anymore) not to tackle this long long overdue security improvement. Please include it in WordPress 6.2.
#129
follow-up:
↓ 130
@
2 years ago
@stgoos fullly agree over here, although considering we are 7.4 already and in fact PHP7 as a whole is EOL by now, so the next version might as well be PHP8+ why not just skip bcrypt and go all-out with argon2id?
#130
in reply to:
↑ 129
@
2 years ago
Replying to my1xt:
@stgoos fullly agree over here, although considering we are 7.4 already and in fact PHP7 as a whole is EOL by now, so the next version might as well be PHP8+ why not just skip bcrypt and go all-out with argon2id?
Valid point, although backwards compatibility for PHP 7.4 for a little longer is something I can understand due to the fact that so many plugins(/themes) haven't been made PHP 8.x compatible yet - unfortunately.
#131
@
2 years ago
A2id has been added to password_hash in 7.3, which means we should be well spaced in terms of supported versions.
going with the WP stats, the amount of wordpresses being on any 6.x is 63% (the most likely who get an update anyway), and PHP7.3+ is above 80%
#132
follow-up:
↓ 133
@
2 years ago
The argon2 suggestion has a problem: It is optional in PHP compilation.
I suggest not using it when compatibility was a concern for a decade.
WordPress always took the stance not to bother people with environment issues and depending on a specific PHP compile-time configuration flag is completely against that notion.
#133
in reply to:
↑ 132
;
follow-up:
↓ 135
@
2 years ago
Replying to bgermann:
The argon2 suggestion has a problem: It is optional in PHP compilation.
I suggest not using it when compatibility was a concern for a decade.
WordPress always took the stance not to bother people with environment issues and depending on a specific PHP compile-time configuration flag is completely against that notion.
That's a perfectly understandable stance from WordPress side.
Is a solution in which bcrypt is used, by default, and argon2 -when detected as available- an idea?
That way we can at least make some progress with this topic after a decade of leaving it aside.
Btw - WordPress stance "to not bother people with environment issues and depending on a specific PHP compile-time configuration flag" could also be turned around. With an estimated 39-43% of all websites on the web running WordPress it could be a very good driver to make the entire internet a safer place. As providers who don't keep their setup up to date with these requirements from WordPress could simply loose business. I know, it's not as black and white as I just described it and quite a few people starting with WordPress / less tech savvy WordPress users on cheaper(?) hosting will probably think it's a WordPress issue rather than a provider issue..., so avoiding that risk (as mentioned I can understand it) leads to topics like this one.
#134
@
2 years ago
that compile flag thing is some next-level backwards junk.
and I certainly see the point of not needing to look for stuff that deep, because extensions can be "easily" seen and asked for, asking your hoster to recompile PHP gets kinda awkward.
#135
in reply to:
↑ 133
@
2 years ago
Replying to stgoos:
Is a solution in which bcrypt is used, by default, and argon2 -when detected as available- an idea?
Is there potentially a library which could be bundled to support argon2 for the (presumably) rare sites which don't have it available?
Supporting two different encryption methods will presumably cause problems when transitioning between the two. Or is there some method to handle this, without forcing a password reset?
#136
@
2 years ago
https://github.com/paragonie/sodium_compat#features-excluded-from-this-polyfill
It's not feasible to polyfill scrypt or Argon2 into PHP and get reasonable performance. Users would feel motivated to select parameters that downgrade security to avoid denial of service (DoS) attacks.
That's from a library which interacts with Argon2. It seems to imply that running a simple PHP based library for Argon2 would not be viable due to performance reasons, and the subsequent issues with DDOS attacks that could occur relating to that.
#137
@
2 years ago
Since WordPress's minimum requirement is PHP 7.4 and Argon2 comes with PHP 7.2 and above, then perhaps it would be appropriate to add in support for Argon2 to WordPress core, and automatically upgrade everyone to Argon2 when they login next. I don't think BCrypt would be needed, since Argon2 is available to all sites which support the WordPress minimum requirements.
There could be a separate plugin and WP CLI tool which could auto-convert the passwords in bulk too. I think there wouldn't be a problem with server overload from the conversion process, but if there was, then we could even implement a system to allow admins to batch convert them all before they upgraded WordPress (leaving the original password hashes in place until core was upgraded).
EDIT: I forgot about the comment above regarding Argon2 being optional during compilation. But isn't a default feature anyway, so someone would need to actively compile with specific settings in order for this not to work right? That doesn't seem like a problem to me. Doesn't WordPress already fail in some ways if PHP is compiled with different settings? (I can't think of any examples off the top of my head, but I'd have assumed such situations would exist) In rare situations where this did crop up, an error could be shown in the admin and the older (current) system could be used instead. If the site was migrated from an Argon2 encrypted site, then users would need to run a password reset in order to regain access.
#138
follow-up:
↓ 139
@
2 years ago
Can somebody enlighten me with a source that the minimum PHP is 7.4 now? I see 5.6 everywhere...
#139
in reply to:
↑ 138
@
2 years ago
Replying to bgermann:
Can somebody enlighten me with a source that the minimum PHP is 7.4 now? I see 5.6 everywhere...
https://wordpress.org/about/requirements/
The minimum recommended is 7.4. The minimum that it technically works with is 5.6. The suggestions above wouldn't nullify that statement, as it could still work with 5.6, just using the current password system.
#140
follow-up:
↓ 141
@
2 years ago
... which still says: "Note: If you are in a legacy environment where you only have older PHP or MySQL versions, WordPress also works with PHP 5.6.20+ and MySQL 5.0+, but these versions have reached official End Of Life and as such may expose your site to security vulnerabilities."
#141
in reply to:
↑ 140
@
2 years ago
areReplying to bgermann:
... which still says: "Note: If you are in a legacy environment where you only have older PHP or MySQL versions, WordPress also works with PHP 5.6.20+ and MySQL 5.0+, but these versions have reached official End Of Life and as such may expose your site to security vulnerabilities."
To me, the "minimum required" is the minimum recommended. Not the minimum that technically still allows it to function. I'm not sure how others interpret that.
#143
in reply to:
↑ 142
@
2 years ago
Replying to bgermann:
I interpret "works with" as "I can certainly log in".
They'd still be able to log in with the suggestion above, albeit they'd need to reset their password if migrating from something with Argon2 to something without it.
This ticket was mentioned in Slack in #core by pikamander2. View the logs.
19 months ago
#145
@
8 months ago
please update to a better password hash than what is currently used and is easy to crack than newer ones.
#146
@
5 months ago
WordPress 6.6 will have minimum PHP 7.2, so Argon2i (not Argon2id which needs 7.3) would be viable if the feature unavailability is handled properly.
This ticket was mentioned in PR #7333 on WordPress/wordpress-develop by @johnbillion.
3 months ago
#147
- Keywords has-unit-tests added
Latest approach to switching from phpass to bcrypt via the native PHP password hashing functions.
Trac ticket: https://core.trac.wordpress.org/ticket/21022
Trac ticket: https://core.trac.wordpress.org/ticket/50027
## Todo
- [ ] Decide on an approach for the 72 character limit imposed by bcrypt
- [ ] Tests for the hash upgrade routine for a user password
- [ ] Tests for the hash upgrade routine for an application password
- [ ] Tests for handling post password hashes
- [ ] Find more todos
#149
@
7 weeks ago
- Milestone changed from Future Release to 6.8
- Owner set to johnbillion
- Status changed from new to accepted
#150
@
3 weeks ago
Alright let's kick this out the door at last. I propose switching to bcrypt for password hashing in WordPress 6.8 using the implementation at https://github.com/WordPress/wordpress-develop/pull/7333 .
The description on that PR covers the points raised in the discussion on this ticket and in #50027. The tl;dr is that we'll switch from using phpass to using password_hash()
and password_verify()
with bcrypt. Loads of info in the PR so please go and take a look and have a read through the description, the FAQ, and the draft for the make/core post.
The one remaining decision to be made concerns the 72 byte input length limit of bcrypt. My proposal is to not implement any specific handling to account for a password greater than 72 bytes in length, and I've written my reasoning for this in the FAQ section of the PR. If there are no objections then we'll go ahead with this.
Feedback, testing, and code reviews on the PR are welcome.
3 weeks ago
#152
I see @johnbillion beat me to the general strategy in his pull request. I think HMAC-SHA512 is a better approach than SHA384, but both are acceptable.
There are libraries that do SHA384 (not HMAC), but then encrypt the password hash with a key. I think the operational security of both approaches is congruent (you need a key to begin password cracking attempts).
In my opinion, doing SHA384 without HMAC, and without encryption, is less beneficial than the other two approaches. However, al 3 are better than raw bcrypt. And bcrypt is a giant leap in security over phpass.
#153
@
3 weeks ago
Please refer OWASP's documentation https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#pre-hashing-passwords-with-bcrypt, they don't recommend pre-hashing passwords when using bcrypt, including use base64 to encoding the sha hash result.
Perhaps we can add input length restrictions to prevent users from using too long passwords?
3 weeks ago
#154
Please refer OWASP's documentation https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#pre-hashing-passwords-with-bcrypt, they don't recommend pre-hashing passwords when using bcrypt.
Perhaps we can add input length restrictions to prevent users from using too long passwords?
@johnbillion commented on PR #7333:
3 weeks ago
#155
I covered that a bit in the FAQ. The main problem is that 72 bytes != 72 characters once you get outside of ASCII, and you'll need active warnings instead of just a maxlength
attribute due to the input masking in browsers.
3 weeks ago
#156
I covered that a bit in the FAQ. The main problem is that 72 bytes != 72 characters once you get outside of ASCII, and you'll need active warnings instead of just a
maxlength
attribute due to the input masking in browsers.
In this case, I prefer to ignore them, like Laravel framework.
Using SHA for pre-hashing will cause more trouble in future upgrades (assuming we need to upgrade to argon2id one day) and will not benefit the vast majority of users and will reduce performance.
3 weeks ago
#157
Using SHA for pre-hashing will cause more trouble in future upgrades (assuming we need to upgrade to argon2id one day) and will not benefit the vast majority of users and will reduce performance.
Can you be specific? What "more trouble" are you envisioning?
3 weeks ago
#158
Using SHA for pre-hashing will cause more trouble in future upgrades (assuming we need to upgrade to argon2id one day) and will not benefit the vast majority of users and will reduce performance.
Can you be specific? What "more trouble" are you envisioning?
Using SHA for pre-hashing will cause more trouble in future upgrades (assuming we need to upgrade to argon2id one day) and will not benefit the vast majority of users and will reduce performance.
Can you be specific? What "more trouble" are you envisioning?
If we upgrade to Argon2 one day, all pre-hashing passwords will need to be re-hashed (Argon2 does not have the 72-byte limit), or we can continue to keep base64+sha384, but this is not necessary in argon2.
If we use PHP's default implementation directly, we won't have these troubles (PHP will handle it automatically).
3 weeks ago
#159
Using SHA for pre-hashing will cause more trouble in future upgrades (assuming we need to upgrade to argon2id one day) and will not benefit the vast majority of users and will reduce performance.
Can you be specific? What "more trouble" are you envisioning?
Using SHA for pre-hashing will cause more trouble in future upgrades (assuming we need to upgrade to argon2id one day) and will not benefit the vast majority of users and will reduce performance.
Can you be specific? What "more trouble" are you envisioning?
If we upgrade to Argon2 one day, all pre-hashing passwords will need to be re-hashed (Argon2 does not have the 72-byte limit), or we can continue to keep base64+sha384, but this is not necessary in argon2.
If we use PHP's default implementation directly, we won't have these troubles (PHP will handle it automatically in future version).
Consider:
if (hash_equals('$2', substr($pwhash, 0, 2)) {
$prehash = base64_encode( hash_hmac( /* ... */ ) );
if (password_verify($prehash, $pwhash)) {
$newHash = password_hash($password, /* ... */).
}
}
3 weeks ago
#160
Another example for reference is https://github.com/laravel/framework/pull/49752 , where they seem to have decided to do nothing.
@johnbillion commented on PR #7333:
3 weeks ago
#161
That's correct. Out of the PHP projects that I listed in the PR, only symfony/password-hasher includes any handling for passwords greater than 72 bytes, and even then it still truncates the resulting hash to due to the use of sha512+base64.
The discussion around hash layering feels dangerously close to rolling your own crypto in order to provide a small amount of additional benefit to an exceptionally small proportion of users.
3 weeks ago
#162
Thankfully, this isn't anywhere near "rolling our own crypto". The SHA2+base64 paradigm has been implemented in several proprietary systems I've worked on over the past decade.
I also cited password_lock above as an open source implementation that does a morally equivalent job. If you need a cryptography expert to sign off on this design, ask the developer of that library to review whatever we propose.
and even then it still truncates the resulting hash due to the use of sha512+base64 which outputs 88 bytes.
SHA-384 and SHA-512 are the exact same algorithm, except SHA-384 has a different initialization vector and is truncated to 384 bits. I don't think it makes any sense to obsess over the truncation here.
If you base64-encode a raw SHA-512 hash to get 88 bytes, and then bcrypt truncates it to a "mere" 72, you still have an input keyspace of 672 or 186 bits of entropy. This is still in far excess of the 128 bits of entropy necessary to be considered secure.
The SHA-2 family of hash functions is secure in most usages (exception: if you're building a MAC by truncating a key and the message). Using HMAC eliminates length-extension, and you can treat HMAC with a secret key as a random oracle.
If you want to be extra paranoid, instead of base64_encode, prefer sodium_bin2base64 if it's available.
3 weeks ago
#163
Thankfully, this isn't anywhere near "rolling our own crypto". The SHA2+base64 paradigm has been implemented in several proprietary systems I've worked on over the past decade.
I also cited password_lock above as an open source implementation that does a morally equivalent job. If you need a cryptography expert to sign off on this design, ask the developer of that library to review whatever we propose.
and even then it still truncates the resulting hash due to the use of sha512+base64 which outputs 88 bytes.
SHA-384 and SHA-512 are the exact same algorithm, except SHA-384 has a different initialization vector and is truncated to 384 bits. I don't think it makes any sense to obsess over the truncation here.
If you base64-encode a raw SHA-512 hash to get 88 bytes, and then bcrypt truncates it to a "mere" 72, you still have an input keyspace of 672 or 486 bits of entropy. This is still in far excess of the 128 bits of entropy necessary to be considered secure.
The SHA-2 family of hash functions is secure in most usages (exception: if you're building a MAC by truncating a key and the message). Using HMAC eliminates length-extension, and you can treat HMAC with a secret key as a random oracle.
If you want to be extra paranoid, instead of base64_encode, prefer sodium_bin2base64 if it's available.
As mentioned before, passwords longer than 72 bytes are rarely used, so it is worth considering whether it is worth doing this.
For sodium_bin2base64
, it is impossible, WP usually only accepts PHP's own implementation (which is why Argon2 is not used, Argon2 requires PHP to use additional compilation parameters).
There is still a long time before 6.8, let's see what other developers think.
3 weeks ago
#165
For
sodium_bin2base64
, it is impossible,
:)
If so, is there a pure PHP implementation of Argon2? Using Argon2 directly would not have these annoying problems.
@johnbillion commented on PR #7333:
3 weeks ago
#166
I've covered argon2 and scrypt in the PR description. Unfortunately there is not.
@johnbillion commented on PR #7333:
3 weeks ago
#168
Again, I've covered this already in the PR description. sodium_compat still requires the optional libsodium extension in order to support argon2 or scrypt. There is no polyfill for either in sodium_compat.
I think we should put a pin in this conversation about the 72 byte limit of bcrypt. It's an interesting computer science problem but it's so far removed from any real life problem that it distracts from assessing the underlying switch to bcrypt.
A 72 byte password has 576 bits of entropy. As soon as a password is over 16 bytes in length, you're beyond the widely recommended 128 bits of entropy to consider a password secure. There is no practical need to retain a higher entropy for passwords greater than 72 bytes.
3 weeks ago
#169
it's so far removed from any real life problem that it distracts from assessing the underlying switch to bcrypt.
I respectfully disagree, for two reasons:
- WordPress has historically bent over backwards to accommodate a small minority of users. This is evident in the minimum PHP bump from 5.2.4 to 5.6 taking so many years after the PHP team stopped supporting it. That only a few users will see a tangible difference has, historically, not been a valid reason to make a security change.
- Bcrypt + SHA2 + Base64, as discussed above, materially improves the security and raises the bar for the PHP ecosystem, making WordPress a leader rather than a follower.
The 72-byte limit isn't the only footgun of vanilla bcrypt. Truncating on NUL characters is the other (which is why eschewing the base64 encoding is a bad move).
That being said, consider users with long passphrases. but where each word in the passphrase is low-entropy (e.g. diceware). They're well-served by this change.
That being said:
I think we should put a pin in this conversation about the 72 byte limit of bcrypt.
That's fine. This is my final word on this topic, unless explicitly queried by another user.
3 weeks ago
#170
it's so far removed from any real life problem that it distracts from assessing the underlying switch to bcrypt.
I respectfully disagree, for two reasons:
- WordPress has historically bent over backwards to accommodate a small minority of users. This is evident in the minimum PHP bump from 5.2.4 to 5.6 taking so many years after the PHP team stopped supporting it. That only a few users will see a tangible difference has, historically, not been a valid reason to make a security change.
- Bcrypt + SHA2 + Base64, as discussed above, materially improves the security and raises the bar for the PHP ecosystem, making WordPress a leader rather than a follower.
The 72-byte limit isn't the only footgun of vanilla bcrypt. Truncating on NUL characters is the other (which is why eschewing the base64 encoding is a bad move).
That being said, consider users with long passphrases. but where each word in the passphrase is low-entropy (e.g. diceware). They're well-served by this change.
That being said:
I think we should put a pin in this conversation about the 72 byte limit of bcrypt.
That's fine. This is my final word on this topic, unless explicitly queried by another user.
The criterion for WordPress to stop supporting PHP is the usage rate (as far as I know, this value is less than 5%).
I think the percentage of users using passwords longer than 72 bytes is much less than 5%.
#171
follow-up:
↓ 172
@
3 weeks ago
I maintain a WordPress plugin does this as well. It intentionally does not pre-hash passwords, and ignore the password length (because it also supports Argon2 with a PHP constant config).
I think the PR looks great as-is, and I really want to vote with the strongest -1 I can muster against pre-hash, pepper, encrypt, or hmac the passwords.
- The point of that plugin is to upgrade to bcrypt, and not to roll our own way of hashing passwords. Totally agreeing and echoing what @johnbillion said in comment:161.
- If we were to pre-hash, we run into a problem that users of the roots' or the plugin I linked above will not be able to uninstall the plugin without and let WordPress core handle the same way these plugins were doing.
- If we were to HMAC or god forbid encrypt passwords, we are not doing ourselves any favor, and might break existing sites because now you have to keep the key in sync with the database backups and replicas. Strong -1.
#172
in reply to:
↑ 171
@
3 weeks ago
Replying to ayeshrajans:
- The point of that plugin is to upgrade to bcrypt, and not to roll our own way of hashing passwords. Totally agreeing and echoing what @johnbillion said in comment:161.
- If we were to pre-hash, we run into a problem that users of the roots' or the plugin I linked above will not be able to uninstall the plugin without and let WordPress core handle the same way these plugins were doing.
Time for WordPress core to adopt the features rollout process as in use with WooCommerce, for existing installations. That way WordPress can draw focus on the additional one-time step(s) that needs to be taken to enable the new feature.
And in all honesty, the problem you described is something WordPress has been creating themselves by leaving this important ticket open for soooo loooong already.
As user of the plugin from Roots, I would not mind informing my users that they will have to set a new password the next time they login due to improved password hashing (/security).
2 weeks ago
#173
I'm with @johnbillion on straight password->bcrypt.
It's counterintuitive, but it will be more secure than password->sha512->bcrypt. Plus, why are we arguing about 470 vs 500+ bits of entropy when ANY form of bcrypt is >30,000,000 more time-consuming to brute force than md5.
The cryptographers who designed bcrypt were well aware it limited possible entropy to ~470 bits. They compromised there on purpose. At modern cost factors it's 1,000's of years of complexity to brute force a 72 character password. But that limitation prevents resource exhaustion DoS attacks on lower-end servers.
Hashing password->base64(sha512)->bcrypt is less secure in several ways. It enables breach correlation / shucking more easily than raw bcrypt. Plus because of the base64 encoding it LOSES entropy from ALL input passwords. This gets dangerous for the large % of users who have 10 char passwords. They start with a max of ~65 bits of entropy, then clipping from 88 -> 72 characters loses some of that already limited entropy. Progressively more pre-bcrypt steps to deal with this clipping just risks opening DoS vectors.
...meanwhile most users (70-80% per HaveIBeenPwned) are using common, short, and/or recycled passwords. If we're really worried about password security we should raise WP's minimum password length, not bikeshed something cryptographers already considered when designing bcrypt.
2 weeks ago
#174
@mbijon Your comment is wrong on several levels, including one I had been ignoring in the previous discussion because I didn't want to derail it with pedantry. Unfortunately, this pedantry is now necessary.
It enables breach correlation / shucking more easily than raw bcrypt. Plus because of the base64 encoding it LOSES entropy from ALL input passwords. This gets dangerous for the large % of users who have 10 char passwords. They start with a max of ~65 bits of entropy, then clipping from 88 -> 72 characters loses some of that already limited entropy. Progressively more pre-bcrypt steps to deal with this clipping just risks opening DoS vectors.
The most obvious evidence against this statement is [(https://medium.com/@rajat29gupta/how-bcrypts-limitations-contributed-to-okta-s-vulnerability-a-lesson-for-developers-39425c644ed5 the real-world security impact of bcrypt's truncation].
It enables breach correlation / shucking more easily than raw bcrypt. Plus because of the base64 encoding it LOSES entropy from ALL input passwords. This gets dangerous for the large % of users who have 10 char passwords.
This is nonsense, which I will explain below.
They start with a max of ~65 bits of entropy, then clipping from 88 -> 72 characters loses some of that already limited entropy.
This is also nonsense, which I will explain below.
Progressively more pre-bcrypt steps to deal with this clipping just risks opening DoS vectors.
The performance impact of a SHA512 hash compared to a password hashing algorithm is negligible. You can benchmark it yourself.
<?php // Initialize up front $start = $stop = microtime(true); // reasonably long example "password" $x = str_repeat('A', 128); $y = ''; $i = 0; $start = microtime(true); for ($i = 0; $i < 10000; ++$i) { $y = hash('sha512', $y, true); } $stop = microtime(true); echo '10,000 iterations of SHA-512 took ', number_format($stop - $start, 3), ' seconds.', PHP_EOL; $start = microtime(true); $y = password_hash($x, PASSWORD_BCRYPT); $stop = microtime(true); echo '1 iteration of bcrypt took ', number_format($stop - $start, 3), ' seconds.', PHP_EOL;
Here's the output on my machine:
10,000 iterations of SHA-512 took 0.004 seconds. 1 iteration of bcrypt took 0.045 seconds.
We aren't doing 10,000 iterations of SHA-512 here, only 1 or 2 (in the case of HMAC).
The DoS risk for this pre-bcrypt hashing isn't real and can't hurt you.
...meanwhile most users (70-80% per HaveIBeenPwned) are using common, short, and/or recycled passwords. If we're really worried about password security we should raise WP's minimum password length, not bikeshed something cryptographers already considered when designing bcrypt.
You... do realize that cryptographers don't like bcrypt, right? That's why the whole Password Hashing Competition existed. We settled on Argon2id and yescrypt. The whole cryptography community was there.
## Revisiting the Misleading Claims About Entropy
The competing proposals looks like this (vaguely):
- Hash (or HMAC) the password with SHA-512.
- SHA-384 is morally congruent to SHA-512, since it's the same exact algorithm but with truncation built-in, and a distinct IV for domain separation. The round functions, word size, and operations are preserved between the two. You don't need to care about this distinction.
- Base64-encode the hash output from step 1.
- Use the output of step 2 with password_hash or password_verify.
SHA-512 is a hash function. Aside from length extension (which isn't relevant to our threat model), you can model SHA-2 family hash functions as a Random Oracle.
SHA-512 maps an input domain of 256(2(63)-1) possible inputs to a mere 512 bits. The input isn't guaranteed to be uniformly random (as they are passwords), but the output is.
Base64-encoding the output of a hash function does not "[enable] breach correlation / shucking more easily than raw bcrypt". At the layer of base64, we have a uniformly random distribution of 512 bits, which gets encoded as slightly more characters from the base64 alphabet. There is no entropy loss at the base64 layer.
The base64 encoding does not, at all, "[lose] entropy from ALL input passwords."
The "entropy" of the password chosen is preserved as long as SHA-512 collisions are computationally infeasible.
One objection to my previous statement might be, "But what about the 88 -> 72 truncation?"
This is *still* secure. 72 characters from the base64 alphabet is still a keyspace representing about 432 bits (or 54 bytes without base64 encoding). This is only 10 bytes short of an untruncated SHA-512, and 6 bytes longer than SHA-384.
The best attack against 432 bits of uncertainty is a birthday collision attack. You'd need about 2216 samples for a 50% chance of a collision (or 2144 samples for a 1/2144 chance of a collision).
Any concerns about the reduction of entropy from the strategies we've been discussing are either rooted in a poor understanding of password-based cryptography, or are bikeshedding over differences that might as well be zero to any WordPress user.
Meanwhile, that Okta breach is a real thing that happened because of bcrypt's truncation. Maybe we should learn from real world incidents?
2 weeks ago
#175
Unrelated, I was notified because of this GitHub Actions workflow.
The following contributors have not linked their GitHub and WordPress.org accounts: @soatok.
I'm not going to even _have_ a WordPress.org account until this abomination goes away:
I recommend disabling this until WP.org is Free again.
2 weeks ago
#176
Stop it @soatok. You're undermining a clear improvement on md5 with incorrect information:
Okta's breach was caused by using bcrypt outside it's design params as a cache key generator. They prefixed heavily in a way that consumed most of the 72 bytes in bcrypt's input. This PR does the right thing and uses password-only as input and without hashing, as recommended by both OWASP and NIST.
The hobbyist community's concern over collisions between two 73-char passwords passed directly to bcrypt is wrong in two ways:
- Most of the examples will have an example where bcrypting 73 chars like
aaa...aaa1
andaaa...aaa2
produce the same output. That's some userland mess that doesn't happen as long as you don't pass a salt toPASSWORD_BCRYPT
. Because bcrypt internally salts with Blowfish randoms, two users can use the SAME password of ANY length and get different bcrypt outputs. - Anyone using 73+ char passwords is using a generator with some level of randomness and not
aaa...
,123...123...
or anything remotely simple.
SHA hashing a password does not increase entropy beyond the original password's. SHA may randomize the characters but there's a symbolic (1:1) correlation between those characters and a low-entropy password. Clipping the length of a base64'd string does matter. It reduces the space to store the shortened SHAs of all 10-char passwords.
Regarding cryptographers, NIST does approve of bcrypt and will for several more years. (...it's looking to be past 2029 before FIPS 140-3 validates any modules that might not include bcrypt. More like 2032-2035 if this DOGE department cuts NIST funding).
You are right that Argon2id would be preferable. However the way to make that happen is to push it to the PHP community and not the WP community. md5 however, has been deprecated well over a decade and @johnbillion's approach is the simplest, most recommended way to implement bcrypt.
2 weeks ago
#177
Stop it @soatok. You're undermining a clear improvement on md5 with incorrect information:
- It's generally considered rude to tell strangers to stop "it". Whatever "it" is, in this context. You're nobody's boss here.
- Regardless of the choice (which is a one-way door): bcrypt with SHA+base64, or plain bcrypt both materially improve over phpass.
Even if WordPress decides against my recommendations, it's worth discussing the facts so it's an informed decision.
I had rested my case until you came along and posted falsehoods. (Accusing me of "incorrect information" is funny.)
SHA hashing a password does not increase entropy beyond the original password's.
I didn't say it did.
Using SHA2 to hash a password permutes the bits through the entire hash output, which means the reduction of the base64-encoded hash output is not a meaningful security reduction. That is my argument.
I made no claims about the entropy of the input domain, only the output.
SHA may randomize the characters but there's a symbolic (1:1) correlation between those characters and a low-entropy password.
This is a meaningless distinction, with my previous statement in mind.
Clipping the length of a base64'd string does matter. It reduces the space to store the shortened SHAs of all 10-char passwords.
I genuinely do not understand this argument.
If you take a raw SHA-512 hash and encode it with base64, there is no entropy loss due to the encoding of the raw hash, and the truncation of such a large hash still has a sufficiently enormous probability space that it doesn't help attackers.
The best possible attack against a truncated SHA-512 is a collision attack. I provided the risk for bcrypt-sha512+base64: After 2144 passwords, there is a 2-144 probability of a collision.
For vanilla bcrypt, it's 2192 for 2-192.
If you _really_ want to argue that 2-192 matters more than 2-144, I invite you to write a paper for your argument and submit it to a journal for an IACR-associated event (e.g., Real World Cryptography) and then share your inevitable rejection letter publicly.
Analyzing how the algorithms transform data can tell you if there is a meaningful security reduction in the steps being taken. The short answer is: No. The truncation of an encoded SHA-512 hash is still longer (and of the same quality) as a SHA-384 hash, which is what the US government recommends in CNSA. If we were truncating to less than 256 bits, there might be cause for concern, but that's not the recommendation here.
A weak, 10-character password is just as weak whether or not you use SHA-512+base64 as described above. No meaningful weakness is introduced by this process, and it prevents one that has broken real-world systems.
Okta's breach was caused by using bcrypt outside it's design params as a cache key generator. They prefixed heavily in a way that consumed most of the 72 bytes in bcrypt's input. This PR does the right thing and uses password-only as input and without hashing, as recommended by both OWASP and NIST.
The point of writing misuse-resistant cryptography (which is what the bcrypt with SHA-512+base64 is) is that you cannot envision all of the possible ways that an API will be used.
Okta previously used bcrypt in a way that wasn't recommended by experts. Developers do this all the time. See also: JWT libraries that accept alg=none.
If someone decides to write a WordPress plugin that uses the WordPress password hashing code for application passwords, and encodes the passwords as email address || password
, and users supply a very long email address:
- With phpass, their passwords won't be hashed with a memory-hard password hash which sucks. That's the situation today.
- With bcrypt, changing the WordPress password hashing class to vanilla bcrypt will introduce a vulnerability that didn't previously exist, due to the 72 character truncation.
- With bcrypt-SHA512+base64, you get the best of both worlds.
You're arguing for the second state, I'm saying the third is the best of both worlds.
In order for it to be correct that I'm "undermining a clear improvement on md5 with incorrect information", I would have to a) be arguing for phpass to continue and b) be stating falsehoods.
Regarding cryptographers, NIST does approve of bcrypt and will for several more years.
This is incorrect. Bcrypt is not a NIST recommend algorithm, and you cannot use it in a FIPS module. You have to use PBKDF2 with a NIST-approved hash function.
NIST does vaguely recommend a "memory hard" function, but they go on to reference Balloon, not bcrypt.
(...it's looking to be past 2029 before FIPS 140-3 validates any modules that might not include bcrypt. More like 2032-2035 if this DOGE department cuts NIST funding).
You are right that Argon2id would be preferable. However the way to make that happen is to push it to the PHP community and not the WP community.
Right. It's a trade-off. I'm not pushing for Argon2id today, I'm telling you what cryptographers prefer today. I know this because cryptography is my job.
Aside: I'm not the first person to recommend this construction. It's had nearly a decade of scrutiny, even if you only look at the PHP community.
It's comical to suggest that vanilla bcrypt is safer than the construction I recommend.
@paulkevan commented on PR #7333:
2 weeks ago
#178
Testing
There's good test coverage for this change. Here are some manual testing steps that can be taken.
Remaining logged in after the update
We should try testing this with wp session too, both the standard version and any extended implementation if possible - the expectation is that they aren't affected (user would remain logged in), but it wouldn't harm to check.
Note that when a user logs in, WordPress already writes to the database via an UPDATE query on the usermeta table to store their updated user session information in the session_tokens meta field. This happens every time a user logs in.
@johnbillion and I also discussed the potential result of this on large datasets, and assumed it wouldn't result in much higher load, and that a bulk process to update this wouldn't be possible due to how passwords are stored/retrieved. More view on this welcome though!
2 weeks ago
#179
Another example for reference is laravel/framework#49752 , where they seem to have decided to do nothing.
I made this Laravel PR to put a bcrypt length check into a validation step, rather than having it just truncate silently, but that was rejected too.
2 weeks ago
#180
Meta comment: I've updated my comment earlier in the thread to clarify one of my statements. I wanted to call attention to this in case it is overlooked.
13 days ago
#181
Having read and reviewed this entire PR, I agree with @johnbillion here with retaining the original bcrypt implementation and limitations. Initially I was onboard with SHA384+bcrypt, but after reading, I agree with just bcrypt.
I would suggest that if using bcrypt for >72char is deemed inappropriate, perhaps it should fall back to the current phpass stretched-md5 hashing method, especially if no consensus can be achieved.
That would result in maximum compatibility for existing WordPress users who use the Password hashes outside of WordPress, while also not introducing yet-another-custom-hash into the web where it's not overly obviously necessary, but while still gaining the bcrypt advantages for where it's possible.
13 days ago
#182
I would suggest that if using bcrypt for >72char is deemed inappropriate, perhaps it should fall back to the current phpass stretched-md5 hashing method, especially if no consensus can be achieved.
I strongly advise against doing this.
While it's generally assumed that the entropy of a 72+ character password is sufficiently good that nobody is going to crack them, even if it's a weaker password hash like phpass, doing so would kill any chance of a world where everyone is on bcrypt or better.
If possible, I'd suggest trying to measure if this is a real problem before committing to it.
/**
* Is this string longer than the target length?
* Avoids side-channels.
* @param string $input
* @param int $targetLength
* @return bool true if the length of $input exceeds $targetLength
*/
function isLongerThanCT(string $input, int $targetLength): bool
{
$amount = PHP_INT_SIZE === 8 ? 63 : 31;
$len = strlen($input); // use mb_strlen($input, '8bit'); on older PHP
$res = (($targetLength - $len) >> $amount) & 1;
/*
* It's worth taking a moment to explain what the hell the line above is doing.
*
* ($targetLength - $len) will return a negative value if $targetLength is larger than $len.
*
* By two's complement, negative values when shifted 31 or 63 places to the right will have a lower
* bit set. We then use a bit mask of `1` and the bitwise `&` operator on the result of the bitshift.
*
* End result? It's the same as if we did the following, except constant-time:
* $res = (int) ($len > $targetLength);
*/
return (bool) ($res);
}
This function returns true
if the string has a length greater than or equal to the target length. This calculation is performed without branches or inequality operators and should therefore have a constant-time execution.
You can use this to measure if the length of users' passwords exceeds some threshold.
Demo: https://3v4l.org/ebQoq
11 days ago
#183
Please @dd32 let’s not fallback to any form of md5 hashing. The form of md5 stretching in PHPass isn’t built to consume time and can’t be configured to extend the time (rounds). Also, it creates a herd vulnerability on large sites like wordpress.com or WPEngine because it doesn’t have per-user salts (it only peppers passwords with what’s named a “SALT” in wp-config). Plus, there are already enough vulnerabilities in md5 for it to be considered broken since 2008.
Any not-broken form of bcrypt password storage (ie: not like Okta’s old implementation) is more secure than any form of md5 storage.
#184
@
8 days ago
There are no coverage tests regarding effects against WP core as a whole, let we check how XMLRPC system.multicall would behave from DoS or application passwords from TTB perspective.
Here are simple benchmark results
WordPress Hash: Time Taken: 0.00169 seconds Hash: $P$Bzw6UBhVjz0K5QWG.iHG52g2lrA7dS0 Bcrypt (Cost 10): Time Taken: 0.064304 seconds Hash: $2y$10$c20BhLvwDSRjNZESRZsbtuu1IA5f7o7Q.BbxmG2Tzofm5C2dI87jC Bcrypt (Cost 12): Time Taken: 0.218802 seconds Hash: $2y$12$b9tSyf0J4bfHn/nhW/8n.u0QEYXCgvMBrILEjjXogQF0D4gvZn8Ca
for the following pseudo code
$p = "1234567890"; wp_hash_password($p); //vs password_hash($p, PASSWORD_BCRYPT, ['cost'=>10]); //vs password_hash($p, PASSWORD_BCRYPT, ['cost'=>12]);
Bcrypt with cost 12 will become/it is default in PHP 8.4. I understand we have no control over PHP, but critical changes/patches like this should be audited with backward and forward compatibility in mind against every existing (maybe against future/planned) functionality in the core.
raphaelahrens commented on PR #7333:
40 hours ago
#185
FYI this was just merged.
https://github.com/OWASP/CheatSheetSeries/pull/1551#pullrequestreview-2489258144
raphaelahrens commented on PR #7333:
40 hours ago
#186
FYI this was just merged.
https://github.com/OWASP/CheatSheetSeries/pull/1551#pullrequestreview-2489258144
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.