9
\$\begingroup\$

I wrote a PHP function that generates salts for use with password hashing:

function getSalt() {
     $charset = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789/\\][{}\'";:?.>,<!@#$%^&*()-_=+|';
     $randString = "";
     $randStringLen = 64;

     while(strlen($randString) < $randStringLen) {
         $randChar = substr(str_shuffle($charset), mt_rand(0, strlen($charset)), 1);
         $randString .= $randChar;
     }

     return $randString;
}

To me, this seems like a secure salt generator because the use of the while() loop and string concatenation for every iteration of substr() and str_shuffle will potentially use the same character in $charset twice (as opposed to simply str_shuffle()'ing once).

\$\endgroup\$

2 Answers 2

10
\$\begingroup\$

It is not necessary for a salt to be perfectly random, it just should be unique for every password you save to defeat rainbow tables and and cracking several passwords at the same time. If it would be required to be perfectly random you would have to use a secure random number generator like openssl_random_pseudo_bytes.


That being said your algorithm looks overly complex to me. As you add a new character to your salt in every iteration of the loop you can use a for loop here: for ($i = 0; $i < $randStringLen; $i++).

There also is no need for the str_shuffle, you are picking a random index from your character set anyway. Note that I have to subtract one from the string length, as everything is zero indexed here. In your version it did not matter, as you are not looping for a fixed number of iterations, but until your string is long enough. I would consider it a bug none-the-less. So this can be reduced to a simple:

substr($charset, mt_rand(0, strlen($charset) - 1), 1);

or even

$charset[mt_rand(0, strlen($charset) - 1)];

Further I suggest reordering your variables. Currently the “internal parameters” are interleaved with the return value (randString). I would move the return value down.

All in all your algorithm would look something like this:

function getSalt() {
     $charset = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789/\\][{}\'";:?.>,<!@#$%^&*()-_=+|';
     $randStringLen = 64;

     $randString = "";
     for ($i = 0; $i < $randStringLen; $i++) {
         $randString .= $charset[mt_rand(0, strlen($charset) - 1)];
     }

     return $randString;
}

But in the end it does not matter how your salt generation algorithm looks like, as you should be using password_hash for the exact reason that you do not have to worry about these kind of issues!

\$\endgroup\$
6
  • \$\begingroup\$ Thank you for the direct critique without jumping straight to "use password_hash() for this!". I do understand that every salt should be unique and not just random, but the random factor plays a part in a salt's uniqueness. What is the advantage of using the for() loop over the while() loop in this scenario? I was looking into password_hash() on PHP.net's manual and the reason I was reluctant to use that right away was because I want to store the salt in the database. From the looks of it, password_hash() returns the salt used, but how exactly would that salt be stored then? \$\endgroup\$
    – Matthew U.
    Jun 7, 2015 at 1:11
  • \$\begingroup\$ @MatthewU. You really should use password_hash, though :-) \$\endgroup\$
    – TimWolla
    Jun 7, 2015 at 1:12
  • \$\begingroup\$ Unfortunately I'm using Debian Wheezy in this scenario which currently runs PHP 5.4, so password_hash() is not available for me. After looking into password_hash() examples further, I now have a thorough understanding of it and will be utilizing that now after I upgrade my system and packages. :) \$\endgroup\$
    – Matthew U.
    Jun 7, 2015 at 1:35
  • 1
    \$\begingroup\$ @MatthewU. Use bcrypt then. You can reuse your salt generation function with a different charset. \$\endgroup\$
    – TimWolla
    Jun 7, 2015 at 1:41
  • \$\begingroup\$ @MatthewU. - there is a password-hash-compatibility script for PHP 5.4 (and patched 5.3.7+) by ircmaxell on github. It uses the same "guts" - mcrypt_create_iv() using dev/urand for the salt, bcrypt for the KDF - but the back-and-forth between PHP and the binaries means it's just a little slower. You can use function_exists() to determine whether or not you need to load the script. The salt (and algo and settings for the algo) is stored in the returned hash string. You don't need another column to store the hash; bcrypt can extract it. \$\endgroup\$ Jun 7, 2015 at 7:26
2
\$\begingroup\$
  1. mt_rand is seeded by a 32 bit integer. So there is little point in generating a 384 bit string. You'll still only get 4 billion different salts.
  2. PHP's bcrypt API will ignore anything beyond the first 128 bits and expects a Base64 encoded salt, so your salt isn't usable for that either.
  3. To generate random numbers you can either use mcrypt_create_iv($size, MCRYPT_DEV_URANDOM) or openssl_random_pseudo_bytes or (random_bytes($size) in php 7). Note that with the openssl variant you should check the second parameter to see if you got back good random data.

    Then encode the output with Base64.

  4. Just use the password_hash API (php 5.5) or its polyfill on older versions. Its easy to use and secure.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.