0

I need to generate a 20 character long random string free from any vowels, and thus far I have the following function to carry out the task.

function random_str()
{
    $i = 1;
    $secret = '';

    while($i==1){
        $str = bin2hex(random_bytes(10));

        $str = preg_replace('/[aeiouAEIOU]|\W+/', '', sha1(microtime() . mt_rand() . $str ));
        $secret .= $str;
        if(strlen($secret) >= 20){
            $i = 0;
        }
    }

    return substr($secret, 0, 20);
}

Is this secure? Is there any way it could be improved?

This isn't used in anything live as of yet, but will be used to generate keys for both resetting passwords and for viewing certain parts of a system that normally requires a user to be logged in.

  • 1
    That is not a CSPRNG, that creates a randomly generated then mangled string. What do you mean by 'secure'? Why do you need to remove vowels? That's pretty odd. – schroeder Feb 8 '17 at 10:26
  • 1
    There are standard ways to generate secure tokens. Is there a reason why you aren't using those techniques? – schroeder Feb 8 '17 at 10:30
  • 4
    Also, the resulting string will only contain the following 14 characters: 0123456789bcdf. What alphabet do you want the characters to be from? – Anders Feb 8 '17 at 10:34
  • It seems I have actually butchered this pretty bad, I hadn't noticed that it only returns those characters. I'm not a security professional by any standards, so this is what I have picked up from what I could find online. most thing I found suggested something similar to the above, minus the lack of vowels, which I have tried to work in myself. as far as I can tell the code isn't an infinite loop at least, it works no problem for me. Finally the reason for the lack of vowels is that this will eventually be customer facing, and removing them to avoid profanity is a set requirement – AntlerFox Feb 8 '17 at 10:37
  • 1
    Instead of deciding which characters you don't want, pick a safe set of characters that you do want, then generate random numbers and map them onto that alphabet. Also, if you want to know the answer to "is this safe?" then you have to define your threat model: what, exactly, are you trying to protect against, and what is outside of the scope of what you are doing? – a CVn Feb 8 '17 at 10:53
2

A clean up of your code

This is the same as what you are doing, but written a bit more elegantly:

function random_str()
{
    $secret = '';
    while(strlen($secret) >= 20) {
       $str = bin2hex(random_bytes(10));
       $str = preg_replace('/[aeiouAEIOU]|\W+/', '', sha1(microtime() . mt_rand() . $str));
       $secret .= $str;
    }
    return substr($secret, 0, 20);
}

Notice I have removed the unnecesary $i variable. (I know, not a security issue, but anyway.)

Is it safe?

The code is very convoluted, and takes a rather odd path to solve a not so hard problem. Some issues:

  • You use the hash sha1(microtime() . mt_rand() . $str) when just relying on $str would be enough. Thats where you get the input from a CSPRNG. Adding some non secure randomness (mt_rand) and the time and then hashing it does not improve it.
  • Since you remove vowels from a hex string you end up only using the character 0123456789bcdf. Thats just fourteen - not a lot.
  • It is a bit strange to first generate a string containing characters you don't want and then throw them away, when you could just only generate the ones you want.

In the end, the result is cryptoghraphically secure, since it relies on random_bytes(), that is a CSPRNG. But the quirks of your code and your lack of understanding of it leads to insecurity, e.g. in this case you might end up with less entropy per character than what you would have expected.

A better way to do it

Use the PHP funciton random_int to pick a character from a fixed alphabet at random:

function random_string($length, $alphabet) {
    $result = ''; //Initialize an empty string.
    $max = strlen($alphabet) - 1; //The maximum random number we want to generate.
    for($i = 0; $i < $length; $i++) { //Do once for every character in the string.
        $result .= $alphabet[random_int(0, $max)]; //Add a random letter from the alphabet.
    }
    return $result;
}

The performance could probably be improved by eliminating the string concatenation in the loop, but it is probably not a big issue.

  • As you say, it doesn't really matter because they shouldn't be generating-then-filtering, but I'm trying to think: does the naive concatenation into sha1() instead of using an hmac cause an issue? Since we're not actually feeding in any sort of key, it shouldn't be a problem, right? – Xiong Chiamiov Feb 8 '17 at 16:07
  • @XiongChiamiov While I can not formally prove it, I find it very hard to believe that feeding SHA1 with unpredictable input would produce predictable output. – Anders Feb 8 '17 at 16:09

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.