Skip to content
This repository has been archived by the owner on Jan 13, 2022. It is now read-only.

Remove mb-string dependency #552

Merged
merged 8 commits into from
May 18, 2016
Merged

Conversation

SammyK
Copy link
Contributor

@SammyK SammyK commented Feb 9, 2016

Re #546

This kills the unnecessary mb-string dependency. In doing so I was able to rip out quite a bit of code & tests related to the overly-complicated cURL client implementation. While ripping out mb-string for signed request signature validation, I made a polyfill for the hash_equals() function in PHP 5.6 (which has been vetted by infosec nerds). This allowed me to removed the duplicate code for comparing the CSRF token for redirect logins as well as signed request signature validations.

Also - the tests were failing out of the box when mcrypt, openssl or curl weren't installed. I fixed all that. :)

Make sure to composer dump-autoload before reviewing this one. :)

@@ -12,8 +12,7 @@
}
],
"require": {
"php": ">=5.4.0",
"ext-mbstring": "*"
"php": ">=5.4.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until PHP 7 is under the "allowed failures" section on Travis, could you use "php": "^5.4" please? (also the >= is unrecommended by Composer itself).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Let's fix this issue in a separate PR. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #554 :)

@paragonie-scott
Copy link
Contributor

While removing dependencies is a good thing, if the extension is available, it would be wise to use mb_strlen($str, '8bit'); instead of strlen() because of function overloading.

    function sammy_hash_equals($knownString, $userString)
    {
        $kLen = strlen($knownString);
        $uLen = strlen($userString);
        if ($kLen !== $uLen) {
            return false;
        }
        $result = 0;
        for ($i = 0; $i < $kLen; $i++) {
            $result |= (ord($knownString[$i]) ^ ord($userString[$i]));
        }
        // They are only identical strings if $result is exactly 0...
        return 0 === $result;
    }
// 8 chars but 32 bytes
$hashA = "\xF0\x9D\x92\xB3" . "\xF0\x9D\xA5\xB3" .
         "\xF0\x9D\x92\xB3" . "\xF0\x9D\xA5\xB3" .
         "\xF0\x9D\x92\xB3" . "\xF0\x9D\xA5\xB3" .
         "\xF0\x9D\x92\xB3" . "\xF0\x9D\xA5\xB3";

$hashB = "\xF0\x9D\x92\xB3" . "\xF0\x9D\xA5\xB3" .
         "\xF0\xAD\x9F\xC0" . "\xF0\xAD\x9F\xC0" .
         "\xF0\xAD\x9F\xC0" . "\xF0\xAD\x9F\xC0" .
         "\xF0\xAD\x9F\xC0" . "\xF0\xAD\x9F\xC0";

var_dump(sammy_hash_equals($hashA, $hashB));

If you run this unit test with mbstring.func_overload set to 2, 3, or 7, this will return true. This is useful if an attacker wants to guess a MAC to forge an arbitrary message. (Since this is a global polyfill function, it's not inconceivable that someone would attempt to use this to validate a HMAC in an encryption library in the same application. Hello, chosen-ciphertext attacks.)

The attack strategy requires a little bit of luck (all bytes in the string must coincide with eight 4-byte UTF-8 sequence + the first 8 bytes must be identical). However, this reduces the cost of a birthday attack from ~2^128 (against HMAC-SHA-256) to about 2^65 (assuming 11 bits needed per 4-byte sequence, but the first 64 bits need to all match, and 2^(n/2) is the 50% mark for birthday collisions).

https://en.wikipedia.org/wiki/UTF-8#Description

Add mb_string check for 8-bit functionality
@SammyK
Copy link
Contributor Author

SammyK commented Feb 10, 2016

Thanks @paragonie-scott! Pulled your changes in. :)

@yguedidi
Copy link
Contributor

@SammyK looks good to me!
I'll just suggest to use the new symfony/polyfill repository for PHP polyfills instead of rewriting them. You can require the subpackage "symfony/polyfill-php56": "^v1.1" that provide a polyfill for hash_equals :)

@SammyK
Copy link
Contributor Author

SammyK commented Feb 10, 2016

@yguedidi Yeah I'd rather pull it in via composer as well. But since historically @gfosco has been against composer dependancies, let's try to butter him up for allowing them in v6. :D

@SammyK
Copy link
Contributor Author

SammyK commented Apr 7, 2016

Ping @gfosco. This one is ready to merge in when you get a sec. :)

@SammyK SammyK added this to the 5.1.5 milestone May 18, 2016
@SammyK SammyK merged commit 2c14499 into facebookarchive:master May 18, 2016
@SammyK SammyK deleted the remove-mb-string branch May 18, 2016 21:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants