Make WordPress Core

Opened 2 years ago

Last modified 2 years ago

#56335 new enhancement

use hash_equals to check password hash

Reported by: hanshenrik's profile hanshenrik Owned by:
Milestone: Awaiting Review Priority: normal
Severity: trivial Version:
Component: Security Keywords: has-patch close
Focuses: Cc:

Description (last modified by SergeyBiryukov)

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)

608.diff (998 bytes) - added by hanshenrik 2 years ago.

Download all attachments as: .zip

Change History (5)

@hanshenrik
2 years ago

This ticket was mentioned in PR #3064 on WordPress/wordpress-develop by divinity76.


2 years ago
#1

  • Keywords has-patch added

.. 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

#2 @hanshenrik
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.

Last edited 2 years ago by hanshenrik (previous) (diff)

#3 @SergeyBiryukov
2 years ago

  • Description modified (diff)

#4 @desrosj
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.

Note: See TracTickets for help on using tickets.