SECURITY: Make token comparison constant time

It seems like our token comparison would be vulnerable to timing
attacks. This will take constant time.

Bug: 61346
Change-Id: I2a9e89120f7092015495e638c6fa9f67adc9b84f

diff --git a/includes/User.php b/includes/User.php
index 4b57dd2..0b28f66 100644 (file)
--- a/includes/User.php
+++ b/includes/User.php
@@ -1056,7 +1056,8 @@ class User {
                        # Get the token from DB/cache and clean it up to remove garbage padding.
                        # This deals with historical problems with bugs and the default column value.
                        $token = rtrim( $proposedUser->getToken( false ) ); // correct token
-                       $passwordCorrect = ( strlen( $token ) && $token === $request->getCookie( 'Token' ) );
+                       // Make comparison in constant time (bug 61346)
+                       $passwordCorrect = strlen( $token ) && $this->compareSecrets( $token, $request->getCookie( 'Token' ) );
                        $from = 'cookie';
                } else {
                        // No session or persistent login cookie
@@ -1075,6 +1076,25 @@ class User {
                }
        }
 
+       /**
+        * A comparison of two strings, not vulnerable to timing attacks
+        * @param string $answer the secret string that you are comparing against.
+        * @param string $test compare this string to the $answer.
+        * @return bool True if the strings are the same, false otherwise
+        */
+       protected function compareSecrets( $answer, $test ) {
+               if ( strlen( $answer ) !== strlen( $test ) ) {
+                       $passwordCorrect = false;
+               } else {
+                       $result = 0;
+                       for ( $i = 0; $i < strlen( $answer ); $i++ ) {
+                               $result |= ord( $answer{$i} ) ^ ord( $test{$i} );
+                       }
+                       $passwordCorrect = ( $result == 0 );
+               }
+               return $passwordCorrect;
+       }
+
        /**
         * Load user and user_group data from the database.
         * $this->mId must be set, this is how the user is identified.