Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#29132 closed enhancement (wontfix)

improve hash_equals() introduced in r29382

Reported by: denis-de-bernardy's profile Denis-de-Bernardy Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.0
Component: Security Keywords:
Focuses: Cc:

Description

The hash_equals() function introduced in r29382 can leak the length of the string being compared. A potential improvement would be to add a nop instead of returning false directly, e.g. something like:

function hash_equals( $a, $b ) { 
	    // Do not attempt to "optimize" this. 
	    $a_length = strlen($a);
	    for ( $i = 0; $i < $a_length; $i++ ) { 
	        $result |= isset( $b[$i] ) && ( ord( $a[ $i ] ) ^ ord( $b[ $i ] ) ); 
	    } 
	 
	    return $result === 0; 
} 

Change History (2)

#1 @Denis-de-Bernardy
10 years ago

Obviously, should be:

return $a_length == strlen( $b ) && $result === 0; 

#2 @nacin
10 years ago

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

It looks like that might still leak the length. That said, you're right in that it is pretty easy to make this not leak the length. Some (but not all) other time-constant comparison functions in the PHP world do this.

However, PHP 5.6's implementation specifically does *not* protect the length. Because this is a "compat" layer for a core PHP function, we decided to mirror it as closely as possible, for better or worse. Otherwise, you might be using it thinking it protects the length when by PHP 5.6 it will not.

Incidentally, the length is often known in these situations anyway, and is known for where we are using it.

Note: See TracTickets for help on using tickets.