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

Fix sha alphabet #12

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix sha alphabet #12

wants to merge 3 commits into from

Conversation

ktomk
Copy link

@ktomk ktomk commented Jul 23, 2013

Unix crypt SHA256/SHA512 are using the wrong alphabet for salts.

First some changes to the tests to show that the current implementation is broken.

Then the fixes.

ktomk added 3 commits July 22, 2013 20:21
The test-data-provider provideTestDetect() is missing to provide a
hash that contains characters in the salt out of the base64
range.

Previously only data with salts in the base64 range was provided.
This had left non-base64 ranged salts untested.

Steps done:

- A valid SHA256 hash containing characters in the salt out of the
  base64 range has been added.

For reference the following script shows the hash, what's its
plain is and that it verifies:

    <?php

    $plain  = 'hello';
    $hash   = '$5$' . "\xE4\"|\xF5|\x08\xC8'\xF054:>\x13\xCB\xED"
              . "\$6I5JMX./GN9KGHTtvHwvp3mxkNv/Ni7/jomOEBgsiM.";

    $verify = $hash === crypt($plain, $hash);
    if (!$verify) {
        throw new RuntimeException('PHP crypt() failure.');
    }

    echo "OK";

https://eval.in/38389
The test-data-provider provideTestDetect() is missing to provide a
hash that contains characters in the salt out of the base64
range.

Previously only data with salts in the base64 range was provided.
This had left non-base64 ranged salts untested.

Steps done:

- A valid SHA512 hash containing characters in the salt out of the
  base64 range has been added.

For reference the following script shows the hash, what's its
plain is and that it verifies:

    <?php

    $plain  = 'hello';
    $hash   = '$6$' . "\xC2?<j\x9A\xE0\xC4\xFCK\x8F\xFD\x87csaO" .
              "\$Oca/TbK.iCdURjqXCnoIyDNggbVF1FWwjxxUYRuYm6HAPP" .
              "mQSDxWa3fSgzcPsTyVdjBv4JLBlj4c13YLOpP5f/";

    $verify = $hash === crypt($plain, $hash);
    if (!$verify) {
        throw new RuntimeException('PHP crypt() failure.');
    }

    echo "OK";

https://eval.in/38390
The test-data used in the imlementations:

 - Unit_Password_Implementation_SHA256Test
 - Unit_Password_Implementation_SHA512Test

did only contain salts inside the base64 range. That did
left non-base64 data for salts untested.

This patch adds hash strings that are using a salt
of 16 times chr(1) instead of chr(0). Both in the mock
of the random generator as well as in test-data.
@ktomk
Copy link
Author

ktomk commented Jul 23, 2013

@ircmaxell
Copy link
Owner

Can we fix those failing tests?

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

Successfully merging this pull request may close these issues.

2 participants