Skip to content

Commit

Permalink
minor #14028 [Security] [Core] String utils refactor (sarciszewski, i…
Browse files Browse the repository at this point in the history
…rcmaxell)

This PR was submitted for the 2.7 branch but it was merged into the 2.3 branch instead (closes #14028).

Discussion
----------

[Security] [Core] String utils refactor

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

This supersedes #13984 (it includes it, but also includes additional refactoring).

Since length information is leaked in any case, preventing unnecessary duplication of secrets is important. Since casting will *always* make a copy, we only cast if absolutely necessary. Additionally, appending will create a new copy of the secret, so we avoid doing that, but instead only iterate over the minimum of the two strings.

Commits
-------

45cfb44 Change behavior to mirror hash_equals() returning early if there is a length mismatch
8269589 CS fixing
bdea4ba Prevent modifying secrets as much as possible
76b36d3 Update StringUtils.php
7221efc Whitespace
56ed71c Update StringUtils.php
  • Loading branch information
fabpot committed Mar 25, 2015
2 parents 36948bb + 45cfb44 commit ec4e9d2
Showing 1 changed file with 37 additions and 11 deletions.
48 changes: 37 additions & 11 deletions src/Symfony/Component/Security/Core/Util/StringUtils.php
Expand Up @@ -38,29 +38,55 @@ private function __construct()
*/ */
public static function equals($knownString, $userInput) public static function equals($knownString, $userInput)
{ {
$knownString = (string) $knownString; // Avoid making unnecessary duplications of secret data
$userInput = (string) $userInput; if (!is_string($knownString)) {
$knownString = (string) $knownString;
}

if (!is_string($userInput)) {
$userInput = (string) $userInput;
}


if (function_exists('hash_equals')) { if (function_exists('hash_equals')) {
return hash_equals($knownString, $userInput); return hash_equals($knownString, $userInput);
} }


$knownLen = strlen($knownString); $knownLen = self::safeStrlen($knownString);
$userLen = strlen($userInput); $userLen = self::safeStrlen($userInput);


// Extend the known string to avoid uninitialized string offsets if ($userLen != $knownLen) {
$knownString .= $userInput; return false;
}


// Set the result to the difference between the lengths $result = 0;
$result = $knownLen - $userLen;


// Note that we ALWAYS iterate over the user-supplied length for ($i = 0; $i < $knownLen; $i++) {
// This is to mitigate leaking length information
for ($i = 0; $i < $userLen; $i++) {
$result |= (ord($knownString[$i]) ^ ord($userInput[$i])); $result |= (ord($knownString[$i]) ^ ord($userInput[$i]));
} }


// They are only identical strings if $result is exactly 0... // They are only identical strings if $result is exactly 0...
return 0 === $result; return 0 === $result;
} }

/**
* Return the number of bytes in a string
*
* @param string $string The string whose length we wish to obtain
* @return int
*/
public static function safeStrlen($string)
{
// Premature optimization
// Since this cannot be changed at runtime, we can cache it
static $func_exists = null;
if ($func_exists === null) {
$func_exists = function_exists('mb_strlen');
}

if ($func_exists) {
return mb_strlen($string, '8bit');
}

return strlen($string);
}
} }

0 comments on commit ec4e9d2

Please sign in to comment.