HMAC validation is broken in 6.0.4+ on PHP 5.5 and below

[curtisdf]

Release 6.0.4 introduced a change in HMAC::timingSafeEquals() which uses mb_strlen() to calculate string lengths when available, instead of strlen(). Unfortunately, this seems to have broken the function. The problem only becomes apparent in PHP 5.5 and below, since in 5.6.0 and onward it uses PHP's built-in hash_equals() function instead. Commenting out lines 54-57 in Namshi/JOSE/Signer/OpenSSL/HMAC.php fixes it for me:

    //if (\function_exists('mb_strlen')) {
        //$knownLength = \mb_strlen($known, '8bit');
        //$inputLength = \mb_strlen($input);
    //}

But the original commit by @cirpo which introduced this code indicates that strlen() also has problems. I'm not sure what the real fix should be...?

FWIW, here's how I am generating tokens:

$sessionId = 12345678;  //  (an integer)
$key = '********************************';   // (an MD5 hash)
$algo = 'HS256';

$jws = new SimpleJWS(['alg' => $algo]);
$jws->setPayload([
    'sub' => $sessionId,
    'iss' => 'My Company'
]);
$jws->sign($key);
$token = $jws->getTokenString();

And here is what I'm doing to validate them:

$jws = SimpleJWS::load($token);
if (!$jws->isValid($key, $algo)) {
    throw new \RuntimeException('Token is invalid!');
}

Any thoughts?

[odino]

hey @curtisdf, ouch, sorry to hear that :( would you mind adding a test to reproduce the bug? I thought we already had something that would get us covered on travis, but apparently we missed this. Once we have it, it's easier for us to look into it.

cheers!

[curtisdf]

I tried for over 2 hours tonight to come up with a test that would demonstrate this problem, but for some reason I can't. But it still fails in our codebase using the sample code I gave above.

I've also checked again using PHP 5.6. Inside HMAC::verify() if I let it execute hash_equals(), my code runs just fine. But if I comment out that line and let it call HMAC::timingSafeEquals() instead, it fails. So, these two functions do not produce the same results for all inputs. What I've found is that in some cases (mine, apparently) the following lines of code do not produce the same lengths even though $known and $input are identical:

$knownLength = \mb_strlen($known, '8bit');   //  produces "32"
$inputLength = \mb_strlen($input);           //  produces "23", "24", "27", "28", etc.

even though $known and $input are identical. The PHP docs for mb_strlen() say that if the second input parameter is not given, it defaults to mb_internal_encoding(). In my case, that is "UTF-8". Here are some more data points:

If $known and $input are both equal to: base64_decode("9Q6ItnvuQ+2sGZIYs/Z18C9HfcvXukLIlqDG1YkRiyI=") then mb_strlen($known, '8bit') == 32 and mb_strlen($input) == 24

If $known and $input are both equal to: base64_decode("FqYdoUENJWrB7xteGy9ksw614rU2d0yBSvr5maGxFPA=") then mb_strlen($known, '8bit') == 32 and mb_strlen($input) == 28

If $known and $input are equal to: base64_decode("JcEB50k6u08bK0dqKfppET/f4g7zPQxorMjE7H4CCcc=") then mb_strlen($known, '8bit') == 32 and mb_strlen($input) == 23

That's about the best I can say right now. :-( I hope this provides some meaningful clues.

[curtisdf]

I might have found the smoking gun. I played around a bit with mb_strlen(), and I found that all 3 of the example signatures I gave above come out to mb_strlen() == 32 if mb_internal_encoding() is left as the PHP default, which on my Homebrew/OSX rig is ISO-8859-1. But if I set mb_internal_encoding() to "UTF-8", then I do get the shorter lengths mentioned in my previous post.

So, is your test suite in TravisCI using mb_internal_encoding() == "ISO-8859-1"?

I don't understand why the need to call mb_strlen() once with '8bit' and once with whatever random default charset PHP happens to be using. Is this intended? If not, what should it be here? Should this be hard-coded to "ISO-8859-1"? As in this below?

if (\function_exists('mb_strlen')) {
    $knownLength = \mb_strlen($known, '8bit');
    $inputLength = \mb_strlen($input, 'iso-8859-1');
}

[cirpo]

@curtisdf as far as I know there are no specific reason, I think we just missed the second parameter sorry. We never had this issues even if it's covered by test on php 5.5. The problem here is that the second mb_strlen is missing the 8bit second parameter. Using the 8bit parameter will give you back the string length in byte

[curtisdf]

Awesome, thanks @cirpo!

[cirpo]

@curtisdf I reopened the issue because there is no stable release out with the fix :)

[curtisdf]

Oh. Okay I see.

[cirpo]

@curtisdf could you please try 6.1.1? It should fix the issue. Thanks

[curtisdf]

Works for me. Thanks @cirpo!! :-D I really appreciate your responsiveness over the weekend. One never knows when working with 3rd-party libraries how fast they will be able to respond if any issues come up.

[odino]

@curtisdf that's because @cirpo is a bot, not a real user :D

[razanbilwani]

:laughing:

[cirpo]

LOL. @curtisdf thanks also to @odino that created the tag and took care about other prs as well. #yalla

[curtisdf]

Ha ha, you guys are funny. :-P I've been accused of being a bot myself too.

[odino]

closing this now :) thanks guys