Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypt-blowfish returns *0 rather than hash #7420

Closed
photodude opened this issue Oct 16, 2016 · 7 comments
Closed

crypt-blowfish returns *0 rather than hash #7420

photodude opened this issue Oct 16, 2016 · 7 comments

Comments

@photodude
Copy link
Contributor

photodude commented Oct 16, 2016

Expected this issue to have been fixed as noted in #1071
Still getting *0 as return

HHVM Version

3.15.2
3.16.0-dev

Standalone code, or other way to reproduce the problem

<?php
    function getCryptedPassword($plaintext, $salt = '', $encryption = 'crypt-blowfish', $show_encrypt = false)
    {
        // mimic the getSalt function so we can use it in the example.
        if ($salt)
        {
            $salt = substr(preg_replace('|^{crypt}|i', '', $salt), 0, 16);
        }
        else
        {
            if (function_exists('random_bytes'))
            {
                $salt = '$2$' . substr(md5(random_bytes(16)), 0, 12) . '$';
            }
            else
            {
                // Fake the salt result (a previously generated salt using https://github.com/paragonie/random_compat )
                $salt = '$2$9936b047ea8b$';
            }
        }
        // Encrypt the password.
        switch ($encryption)
        {
            case 'plain':
                return $plaintext;
            case 'sha':
                $encrypted = base64_encode(mhash(MHASH_SHA1, $plaintext));
                return ($show_encrypt) ? '{SHA}' . $encrypted : $encrypted;
            case 'crypt':
            case 'crypt-des':
            case 'crypt-md5':
            case 'crypt-blowfish':
                return ($show_encrypt ? '{crypt}' : '') . crypt($plaintext, $salt);
            case 'md5-hex':
            default:
                $encrypted = ($salt) ? md5($plaintext . $salt) : md5($plaintext);
                return ($show_encrypt) ? '{MD5}' . $encrypted : $encrypted;
        }
    }
$plaintext = 'mySuperSecretPassword';

if (function_exists('random_bytes'))
{
    $salt = '$2$' . substr(md5(random_bytes(16)), 0, 12) . '$';
}
else
{
    // Fake the salt result (a randomly created salt using https://github.com/paragonie/random_compat )
    $salt = '$2$9936b047ea8b$';
}

echo strlen(crypt($plaintext, $salt)) . "\n";
echo crypt($plaintext, $salt) . "\n";
echo strlen(getCryptedPassword($plaintext, $salt, 'crypt-blowfish')) . "\n";
echo getCryptedPassword($plaintext, $salt, 'crypt-blowfish');

More test code from https://github.com/facebook/hhvm/blob/master/hphp/test/zend/bad/ext/standard/tests/strings/crypt_blowfish_variation2.php

<?php
$crypt = crypt(b'U*U', b'$2a$CCCCCCCCCCCCCCCCCCCCC.E5YPO9kmyuRGyh0XouQYb4YMJKvyOeW');
if ($crypt===b'$2SHYF.wPGyfE') {
    echo "OK\n"; // This is the expected result
} else {
    echo "Not OK\n"; // This is a Failure
}
?>

https://3v4l.org/GOZIG

Expected result

a hash of length 13 should be returned from crypt()

Actual result

return is just *0 a value indicating a failure

@Orvid Orvid self-assigned this Oct 16, 2016
@Orvid
Copy link
Contributor

Orvid commented Oct 16, 2016

I see the cause, will put up a diff tomorrow.

@photodude
Copy link
Contributor Author

Thanks @Orvid I look forward to the fix.

@Orvid
Copy link
Contributor

Orvid commented Oct 17, 2016

Hrmmm... It actually looks like PHP 7 actually changed this behavior: https://3v4l.org/5tIPh

The key that's being provided says it's blowfish (the $2a$), but is then missing the number of rounds of hashing, which is supposed to come immediately after the hash type. $2a$10$ would be a valid prefix.

The behavior of getting a hash back is the result of it falling back to DES in that case, which php/php-src@4a2fe3d changed.

@photodude
Copy link
Contributor Author

@Orvid since the behavior has changed in PHP 7, maybe this issue can be closed as a "won't fix".

From our end it causes a failure in our old password encryption method which was replaced with the newer PHP password hash. The old method is only implemented as a bc for upgrades in the 3.x versions and has been dropped in 4.0-dev.

Thoughts?

@photodude
Copy link
Contributor Author

@Orvid I've run into another issue with this. Apparently, if there is a case needing to verify a crypt() hashed text it will fail.

example code

$password = 'mySuperSecretPassword';
$hash = '$2xzaiMREaf7o';

$valid = hash_equals(crypt($password, $hash), $hash);

if ($valid === true)
{
    echo "OK\n"; // This is the expected result
}
else
{
    echo "Not OK\n"; // This is a Failure
}

https://3v4l.org/LI9VY

Note in php 7.0-7.1 the result is OK with a notice about Deprecated: crypt(): Supplied salt is not valid for DES. Possible bug in provided salt format. where HHVM fails this kind of verification test

@photodude
Copy link
Contributor Author

Still an issue in HHVM 3.19.0
https://3v4l.org/LI9VY

also differs from PHP 7 @Orvid please add the php 7 incompatibility tag here

@photodude
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants