Opened 2 years ago
Last modified 2 years ago
#56335 new enhancement
use hash_equals to check password hash
Reported by: | hanshenrik | Owned by: | |
---|---|---|---|
Milestone: | Awaiting Review | Priority: | normal |
Severity: | trivial | Version: | |
Component: | Security | Keywords: | has-patch close |
Focuses: | Cc: |
Description (last modified by )
today in wp-includes/class-phpass.php under function CheckPassword we find
# This is not constant-time. In order to keep the code simple, # for timing safety we currently rely on the salts being # unpredictable, which they are at least in the non-fallback # cases (that is, when we use /dev/urandom and bcrypt). return $hash === $stored_hash;
and while i agree that a constant-time comparison is probably not needed, it's a trivial change to fix it, and better safe than sorry. I suggest changing it to
if(PHP_VERSION_ID >= 50600){ return hash_equals($stored_hash, $hash); } else { # This is not constant-time. In order to keep the code simple, # for timing safety we currently rely on the salts being # unpredictable, which they are at least in the non-fallback # cases (that is, when we use /dev/urandom and bcrypt). return $hash === $stored_hash; }
PHP_VERSION_ID was introduced in 5.2.7, and i doubt WordPress still need to support PHP5.2. Unsure if WordPress still support 5.5? if the answer is no, the entire PHP_VERSION_ID can be removed.
Attachments (1)
Change History (5)
This ticket was mentioned in PR #3064 on WordPress/wordpress-develop by divinity76.
2 years ago
#1
- Keywords has-patch added
#2
@
2 years ago
turns out PHP<5.6 support has been dropped, that simplifies things, PR updated :)
Also fwiw PHP's built-in password_equals() also use constant-time hash check internally.
#4
@
2 years ago
- Keywords close added
- Version trunk deleted
I'm going to add a close
suggestion here.
The PHPass class is an external library, though WordPress has made some changes to it over time (see the description of #51549).
I believe that the original intent of PHPass was to properly support password hashing on PHP < 5.5, which no longer applies to WordPress. But moving off of PHPass is a much larger discussion currently being had in #50027 and #21022. I think I prefer that we direct any effort and attention towards those tickets instead.
.. on PHP>=5.6.0
notably it would be possible to implement constant time on <5.6.0, but it's definitely not worth the effort. the PHP_VERSION_ID constant was introduced in PHP 5.2.7
Unsure if WordPress still support 5.5? if the answer is no, the entire PHP_VERSION_ID check can be removed.
Trac ticket: See https://core.trac.wordpress.org/ticket/56335