Make WordPress Core

Opened 3 years ago

Closed 7 months ago

#56335 closed enhancement (wontfix)

use hash_equals to check password hash

Reported by: hanshenrik's profile hanshenrik Owned by:
Milestone: 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 3 years ago.

Download all attachments as: .zip

Change History (6)

@hanshenrik
3 years ago

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


3 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
3 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 3 years ago by hanshenrik (previous) (diff)

#3 @SergeyBiryukov
3 years ago

  • Description modified (diff)

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

#5 @johnbillion
7 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Closing in favour of #50027 and #21022. Thanks!

Note: See TracTickets for help on using tickets.