Multiple so called 'type juggling' attacks. Most notably PhabricatorUser::validateCSRFToken() is 'bypassable' in certain cases.

How it was found

/* MONGOOSE mongoose MONGOOSE mongoose MONGOOSE mongoose MONGOOSE mongoose */

The Phabricator code base is at various places vulnerable for so called 'type juggling' 1 attacks. Most notably PhabricatorUser::validateCSRFToken() is 'bypassable' in certain cases.

Type Juggling

Since PHP's loose type comparison operators compare only data values but not their associated types, deriving variable types from context. PHP's string conversion rules 2 specify strings (when evaluated in a numeric context) with leading decimal, hexadecimal, infinity, NAN or radix (a '.') data optionally followed by an exponent are evaluated as floats.

What this means is that a string like 00e13242 is cast to 0 and as such, to PHP 0e94323 == 00e19384.

Translated into code, this means that the following comparison:

(hash($randomSecret) == $user_input)

will return true if hash($randomSecret) returns a hash in the form 0+[eE]\d+ and the $user_input is given as "0". This applies to various hashing algorithms, including (but not limited to) MD5 and SHA1. If you're not convinced, try running the PHP-code at the bottom of this text for a PoC with SHA1.

How this applies to Phabricator

In order to understand how this applies to Phabricator, we need to first know that static method PhabricatorHash::digest() located at src/infrastructure/util/PhabricatorHash.php 3 on line 25 returns a sha1 hash via hash_hmac();

Then we need to know that in AphrontRequest::validateCSRF();4 the static function PhabricatorUser::validateCSRFToken($token); is called with a user-supplied $token.

Now, if we look at PhabricatorUser::validateCSRFToken() located at phabricator/src/applications/people/storage/PhabricatorUser.php 5, on line 409 we see that getRawCSRFToken (which also returns a sha1 hash) is called and it's return value is stored in $valid

$valid = $this->getRawCSRFToken($ii);

then on line 412 we see:

if ($token == $valid) {

or, if the $version is 'breach' we reach this code on line 419-420:

$digest = PhabricatorHash::digest($valid, $salt);
if (substr($digest, 0, self::CSRF_TOKEN_LENGTH) == $token) {

Both these comparisons meets the criteria for type juggling. Since $valid is a randomly generated sha1 hash which eventually will be of the form 0+[eE]\d+ after sufficient regenerations and the same goes for $digest. In addition, $token is a user-supplied value.

TL;DR

So in short, some expected CSRF-tokens will equal a user-supplied CSRF-token containing "0". In essence this is a reduction in entropy of the CSRF-tokens.

to stop PHP from automatically casting either value to another type during comparison, simply change:

if ($token == $valid) {

to

if ($token === $valid) {

And likewise:

if (substr($digest, 0, self::CSRF_TOKEN_LENGTH) == $token) {

to

if (substr($digest, 0, self::CSRF_TOKEN_LENGTH) === $token) {

In addition to the above vulnerability, there are other notable misuses of PHP comparison operators.

phabricator/src/applications/conduit/method/ConduitConnectConduitAPIMethod.php:144 in protected function execute(ConduitAPIRequest $request) 6:

    $valid = sha1($token.$user->getConduitCertificate());
    if ($valid != $signature) { 
        throw new ConduitException('ERR-INVALID-CERTIFICATE');

The != operator is not type strict and $signature is user-specified. $token is also user-specified. To fix this != should be changed to !==.

phabricator/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php in verifyMessage() on line 15 and 16 7:

$sig = $request->getStr('signature');
return hash_hmac('sha256', $timestamp.$token, $api_key) == $sig

Where $sig is obviously a user-supplied value and can thus be 0, and again the return value of hash_hmac can be of the form 0+[eE]\d+.

To fix this, change: hash_hmac('sha256', $timestamp.$token, $api_key) == $sig

to: hash_hmac('sha256', $timestamp.$token, $api_key) === $sig

Examples:

• type juggling with sha18

  <?php /* sha1 type juggling PoC values */ $v1 = sha1("AAJd1x3j"); $v2 = sha1("AAPkbYlH"); $v3 = sha1("AAZlIwOZ"); var_dump($v1, $v2, $v3); /* wrong way to compare these hashes */ var_dump( $v1 == $v2, $v2 == $v3, $v3 == $v1, $v1 == "0"); /* correct way */ var_dump( $v1 === $v2, $v2 === $v3, $v3 === $v1 ); ?>

The above will output:

string(40) "00e6811279456694288001763399976992804485"
string(40) "0e51223820731210116366152413868569204545"
string(40) "0e13965443605273185827757762777509208778"
bool(true)  
bool(true)
bool(true)
bool(true)
bool(false)
bool(false)
bool(false)

References:

1. http://turbochaos.blogspot.nl/2013/08/exploiting-exotic-bugs-php-type-juggling.html
2. http://php.net/manual/en/language.types.string.php#language.types.string.conversion
3. https://github.com/phacility/phabricator/blob/master/src/infrastructure/util/PhabricatorHash.php#L25
4. https://github.com/phacility/phabricator/blob/master/src/aphront/AphrontRequest.php#L249
5. https://github.com/phacility/phabricator/blob/master/src/applications/people/storage/PhabricatorUser.php
6. https://github.com/phacility/phabricator/blob/master//src/applications/conduit/method/ConduitConnectConduitAPIMethod.php#L145
7. https://github.com/phacility/phabricator/blob/master/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php#L15
8. https://pen-testing.sans.org/blog/pen-testing/2014/12/18/php-weak-typing-woes-with-some-pontification-about-code-and-pen-testing

/*END MONGOOSE mongoose MONGOOSE mongoose MONGOOSE mongoose MONGOOSE mongoose */