function GenerateRandomString($Length) {
$Length /= 2;
$cstrong = false;
while ($cstrong != TRUE){
$bytes = openssl_random_pseudo_bytes($Length, $cstrong);
}
$hex = bin2hex($bytes);
return $hex;
}
Is this a good function for generating a cryptographically secure random string? Is there a chance, that it'll get into an infinite loop, because $cstrong
stays 'False'?
3 Answers 3
The documentation says that crypto_strong
is set depending on the algorithm used. So it will never go from false to true, so yes, this might result in an infinite loop.
Use it instead to see if you should trust the result. If it is true, the string should be secure.
(you should also never design your own crypto algorithms for anything other than academic interest)
because this is codereview, I also have a couple of notes regarding your code:
- Why do you divide the length by 2? Seems unnecessary. If it does have a purpose, comment on it.
false
TRUE
: use case uniformly. eitherfalse
andtrue
orFALSE
TRUE
$hex = bin2hex($bytes); return $hex;
just make this one line:return bin2hex($bytes)
- it is convention to use camelCase for variables, so
Length
should belength
and the function namegenerateRandomString
-
\$\begingroup\$ The purpose of $Length /= 2; is that I want the Parameter $Length to be the strlength of the output string and not the length of the bytes \$\endgroup\$Entimon– Entimon2014年08月05日 11:55:56 +00:00Commented Aug 5, 2014 at 11:55
-
\$\begingroup\$ Additional suggestion:
do..while
instead ofwhile
. \$\endgroup\$deceze– deceze2014年08月05日 13:29:41 +00:00Commented Aug 5, 2014 at 13:29
GenerateRandomString
is a poor name. I don't know what the contents of my string will be. In your case, you've restricted the character set to[0-9A-F]
, but that's not reflected in the name. Consider renaming this togenerateRandomHexadecimalString
or something that more clearly states the intention.You noted why
$length
is divided by two in a comment on @tim's answer. Since he couldn't determine the reasoning at first glance, this would probably be a good thing to put in a comment in the actual code.This function does not actually generate strings of odd length. To do so, you'll need to rewrite to something functionally equivalent to the following:
function generateRandomHexString($length) { return substr(bin2hex(openssl_random_pseudo_bytes(ceil($length / 2))), 0, $length); }
You should just use https://github.com/paragonie/random_compat. It's a polyfill for the random_bytes()
function in PHP7, and will use the best source of randomness available on your host.
Note that openssl_random_pseudo_bytes()
is not a good source of randomness unless you're running a very new release of PHP. Quoting from ERRATA.md
in the random_compat repository:
Finally, we use openssl_random_pseudo_bytes() as a last resort, due to PHP bug #70014. Internally, this function calls RAND_pseudo_bytes(), which has been deprecated by the OpenSSL team. Furthermore, it might silently return weak random data if it is called before OpenSSL's userspace CSPRNG is seeded.
$cstrong
is rare to be FALSE, but some systems may be broken or old. \$\endgroup\$