1
\$\begingroup\$
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'?

\$\endgroup\$
  • \$\begingroup\$ According to the documentation $cstrong is rare to be FALSE, but some systems may be broken or old. \$\endgroup\$ – ragol Aug 5 '14 at 10:36
5
\$\begingroup\$

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. either false and true or FALSE 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 be length and the function name generateRandomString
\$\endgroup\$
  • \$\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 Aug 5 '14 at 11:55
  • \$\begingroup\$ Additional suggestion: do..while instead of while. \$\endgroup\$ – deceze Aug 5 '14 at 13:29
2
\$\begingroup\$
  • 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 to generateRandomHexadecimalString 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);
    }
    
\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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