patternphpModerate
PHP salt generator
Viewed 0 times
phpsaltgenerator
Problem
I wrote a PHP function that generates salts for use with password hashing:
To me, this seems like a secure salt generator because the use of the
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).Solution
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
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
There also is no need for the
or even
Further I suggest reordering your variables. Currently the “internal parameters” are interleaved with the return value (
All in all your algorithm would look something like this:
But in the end it does not matter how your salt generation algorithm looks like, as you should be using
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!Code Snippets
substr($charset, mt_rand(0, strlen($charset) - 1), 1);$charset[mt_rand(0, strlen($charset) - 1)];function getSalt() {
$charset = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789/\\][{}\'";:?.>,<!@#$%^&*()-_=+|';
$randStringLen = 64;
$randString = "";
for ($i = 0; $i < $randStringLen; $i++) {
$randString .= $charset[mt_rand(0, strlen($charset) - 1)];
}
return $randString;
}Context
StackExchange Code Review Q#92869, answer score: 10
Revisions (0)
No revisions yet.