PleaseStand has uploaded a new change for review. https://gerrit.wikimedia.org/r/168931
Change subject: hash_equals(): Avoid division by zero when $known_string is empty ...................................................................... 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 (cherry picked from commit 4620e3b862568d76661e86857779795f4f974e13) --- M includes/GlobalFunctions.php 1 file changed, 24 insertions(+), 13 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core refs/changes/31/168931/1 diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php index 490df24..27f7cac 100644 --- a/includes/GlobalFunctions.php +++ b/includes/GlobalFunctions.php @@ -102,19 +102,30 @@ } // 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 @@ 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 ); -- To view, visit https://gerrit.wikimedia.org/r/168931 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I1b347e69b39af3d7d8ba6673af63f1a616befbdf Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/core Gerrit-Branch: REL1_24 Gerrit-Owner: PleaseStand <pleasest...@live.com> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
