sync svn r23088 "Implements fixed time string comparisons between pot…

…entially raw user input and sensitive data such as plaintext passwords and/or message digests"
chematm · Oct 12, 2011 · 96c2571 · 96c2571
1 parent 563685d
commit 96c2571
Show file tree

Hide file tree

Showing 4 changed files with 95 additions and 6 deletions.
diff --git a/library/Zend/Authentication/Adapter/Digest.php b/library/Zend/Authentication/Adapter/Digest.php
@@ -206,7 +206,7 @@ public function authenticate()
 
         while ($line = trim(fgets($fileHandle))) {
             if (substr($line, 0, $idLength) === $id) {
-                if (substr($line, -32) === md5("$this->_username:$this->_realm:$this->_password")) {
+                if ($this->_secureStringCompare(substr($line, -32), md5("$this->_username:$this->_realm:$this->_password"))) {
                     $result['code'] = AuthenticationResult::SUCCESS;
                 } else {
                     $result['code'] = AuthenticationResult::FAILURE_CREDENTIAL_INVALID;
@@ -220,4 +220,26 @@ public function authenticate()
         $result['messages'][] = "Username '$this->_username' and realm '$this->_realm' combination not found";
         return new AuthenticationResult($result['code'], $result['identity'], $result['messages']);
     }
+
+    /**
+     * Securely compare two strings for equality while avoided C level memcmp()
+     * optimisations capable of leaking timing information useful to an attacker
+     * attempting to iteratively guess the unknown string (e.g. password) being
+     * compared against.
+     *
+     * @param string $a
+     * @param string $b
+     * @return bool
+     */
+    protected function _secureStringCompare($a, $b)
+    {
+        if (strlen($a) !== strlen($b)) {
+            return false;
+        }
+        $result = 0;
+        for ($i = 0; $i < strlen($a); $i++) {
+            $result |= ord($a[$i]) ^ ord($b[$i]);
+        }
+        return $result == 0;
+    }
 }
diff --git a/library/Zend/Authentication/Adapter/Http.php b/library/Zend/Authentication/Adapter/Http.php
@@ -504,7 +504,8 @@ protected function _basicAuth($header)
         }
 
         $password = $this->_basicResolver->resolve($creds[0], $this->_realm);
-        if ($password && $password == $creds[1]) {
+        if ($password && 
+            $this->_secureStringCompare($password, $creds[1])) {
             $identity = array('username'=>$creds[0], 'realm'=>$this->_realm);
             return new Authentication\Result(Authentication\Result::SUCCESS, $identity);
         } else {
@@ -594,7 +595,7 @@ protected function _digestAuth($header)
 
         // If our digest matches the client's let them in, otherwise return
         // a 401 code and exit to prevent access to the protected resource.
-        if ($digest == $data['response']) {
+        if ($this->_secureStringCompare($digest, $data['response'])) {
             $identity = array('username'=>$data['username'], 'realm'=>$data['realm']);
             return new Authentication\Result(Authentication\Result::SUCCESS, $identity);
         } else {
@@ -802,4 +803,26 @@ protected function _parseDigestAuth($header)
 
         return $data;
     }
+
+    /**
+     * Securely compare two strings for equality while avoided C level memcmp()
+     * optimisations capable of leaking timing information useful to an attacker
+     * attempting to iteratively guess the unknown string (e.g. password) being
+     * compared against.
+     *
+     * @param string $a
+     * @param string $b
+     * @return bool
+     */
+    protected function _secureStringCompare($a, $b)
+    {
+        if (strlen($a) !== strlen($b)) {
+            return false;
+        }
+        $result = 0;
+        for ($i = 0; $i < strlen($a); $i++) {
+            $result |= ord($a[$i]) ^ ord($b[$i]);
+        }
+        return $result == 0;
+    }
 }
diff --git a/library/Zend/InfoCard/XML/Security.php b/library/Zend/InfoCard/XML/Security.php
@@ -154,7 +154,7 @@ static public function validateXMLSignature($strXMLInput)
 
         $transformed_xml_binhash = pack("H*", sha1($transformed_xml));
 
-        if($transformed_xml_binhash != $dValue) {
+        if (!$this->_secureStringCompare($transformed_xml_binhash, $dValue)) {
             throw new Security\Exception\RuntimeException("Locally Transformed XML does not match XML Document. Cannot Verify Signature");
         }
 
@@ -276,4 +276,26 @@ static protected function _encodeValue($data, $type)
 
         throw new Security\Exception\RuntimeException("Invalid code path");
     }
+
+    /**
+     * Securely compare two strings for equality while avoided C level memcmp()
+     * optimisations capable of leaking timing information useful to an attacker
+     * attempting to iteratively guess the unknown string (e.g. password) being
+     * compared against.
+     *
+     * @param string $a
+     * @param string $b
+     * @return bool
+     */
+    static protected function _secureStringCompare($a, $b)
+    {
+        if (strlen($a) !== strlen($b)) {
+            return false;
+        }
+        $result = 0;
+        for ($i = 0; $i < strlen($a); $i++) {
+            $result |= ord($a[$i]) ^ ord($b[$i]);
+        }
+        return $result == 0;
+    }
 }
diff --git a/library/Zend/OpenId/Provider/GenericProvider.php b/library/Zend/OpenId/Provider/GenericProvider.php
@@ -770,12 +770,34 @@ protected function _checkAuthentication($version, $params)
                 $data .= $params['openid_' . strtr($key,'.','_')]."\n";
             }
         }
-        if (base64_decode($params['openid_sig']) ===
-            OpenId\OpenId::hashHmac($macFunc, $data, $secret)) {
+        if ($this->_secureStringCompare(base64_decode($params['openid_sig']),
+            OpenId\OpenId::hashHmac($macFunc, $data, $secret))) {
             $ret['is_valid'] = 'true';
         } else {
             $ret['is_valid'] = 'false';
         }
         return $ret;
     }
+
+    /**
+     * Securely compare two strings for equality while avoided C level memcmp()
+     * optimisations capable of leaking timing information useful to an attacker
+     * attempting to iteratively guess the unknown string (e.g. password) being
+     * compared against.
+     *
+     * @param string $a
+     * @param string $b
+     * @return bool
+     */
+    protected function _secureStringCompare($a, $b)
+    {
+        if (strlen($a) !== strlen($b)) {
+            return false;
+        }
+        $result = 0;
+        for ($i = 0; $i < strlen($a); $i++) {
+            $result |= ord($a[$i]) ^ ord($b[$i]);
+        }
+        return $result == 0;
+    }
 }