From 4620e3b862568d76661e86857779795f4f974e13 Mon Sep 17 00:00:00 2001 From: Kevin Israel Date: Tue, 24 Jun 2014 01:33:46 -0400 Subject: [PATCH] hash_equals(): Avoid division by zero when $known_string is empty Per Tim Starling's review of Icb239471, reverted back to the version of the function from Patch Set 1 of Iece006ec, which did not have the bug. This version does not attempt to minimize the inevitable leakage of the string's length. Also revised the doc comment to explain more effectively what the problem with a normal (===) string comparison is for the use cases of this function. Follows-up b9e1d5f5c066. Change-Id: I1b347e69b39af3d7d8ba6673af63f1a616befbdf --- includes/GlobalFunctions.php | 37 +++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php index 3306acda3e..03bdd767c2 100644 --- a/includes/GlobalFunctions.php +++ b/includes/GlobalFunctions.php @@ -102,19 +102,30 @@ if ( !function_exists( 'gzdecode' ) ) { } // hash_equals function only exists in PHP >= 5.6.0 +// http://php.net/hash_equals if ( !function_exists( 'hash_equals' ) ) { /** - * Check whether a user-provided string is equal to a fixed-length secret without - * revealing bytes of the secret through timing differences. + * Check whether a user-provided string is equal to a fixed-length secret string + * without revealing bytes of the secret string through timing differences. * - * This timing guarantee -- that a partial match takes the same time as a complete - * mismatch -- is why this function is used in some security-sensitive parts of the code. - * For example, it shouldn't be possible to guess an HMAC signature one byte at a time. + * The usual way to compare strings (PHP's === operator or the underlying memcmp() + * function in C) is to compare corresponding bytes and stop at the first difference, + * which would take longer for a partial match than for a complete mismatch. This + * is not secure when one of the strings (e.g. an HMAC or token) must remain secret + * and the other may come from an attacker. Statistical analysis of timing measurements + * over many requests may allow the attacker to guess the string's bytes one at a time + * (and check his guesses) even if the timing differences are extremely small. + * + * When making such a security-sensitive comparison, it is essential that the sequence + * in which instructions are executed and memory locations are accessed not depend on + * the secret string's value. HOWEVER, for simplicity, we do not attempt to minimize + * the inevitable leakage of the string's length. That is generally known anyway as + * a chararacteristic of the hash function used to compute the secret value. * * Longer explanation: http://www.emerose.com/timing-attacks-explained * * @codeCoverageIgnore - * @param string $known_string Fixed-length secret to compare against + * @param string $known_string Fixed-length secret string to compare against * @param string $user_string User-provided string * @return bool True if the strings are the same, false otherwise */ @@ -134,14 +145,14 @@ if ( !function_exists( 'hash_equals' ) ) { return false; } - // Note that we do one thing PHP doesn't: try to avoid leaking information about - // relative lengths of $known_string and $user_string, and of multiple $known_strings. - // However, lengths may still inevitably leak through, for example, CPU cache misses. $known_string_len = strlen( $known_string ); - $user_string_len = strlen( $user_string ); - $result = $known_string_len ^ $user_string_len; - for ( $i = 0; $i < $user_string_len; $i++ ) { - $result |= ord( $known_string[$i % $known_string_len] ) ^ ord( $user_string[$i] ); + if ( $known_string_len !== strlen( $user_string ) ) { + return false; + } + + $result = 0; + for ( $i = 0; $i < $known_string_len; $i++ ) { + $result |= ord( $known_string[$i] ) ^ ord( $user_string[$i] ); } return ( $result === 0 ); -- 2.20.1