How secure is this JavaScript password generator?

Question

I wonder how secure is this JavaScript password generator:

var Password = {

  _pattern : /[a-zA-Z0-9_\-\+\.\!\@\#\$\%\^\&\*\(\)\_\+\{\}\:\"\<\>\?\|\[\]\;\'\,\.\/\`\~\']/,

  _getRandomByte : function()
  {

    if(window.crypto && window.crypto.getRandomValues) 
    {
      var result = new Uint8Array(1);
      window.crypto.getRandomValues(result);
      return result[0];
    }
    else if(window.msCrypto && window.msCrypto.getRandomValues) 
    {
      var result = new Uint8Array(1);
      window.msCrypto.getRandomValues(result);
      return result[0];
    }
    else
    {
      return Math.floor(Math.random() * 256);
    }
  },

  generate : function(length)
  {
    return Array.apply(null, {'length': length})
      .map(function()
      {
        var result;
        while(true) 
        {
          result = String.fromCharCode(this._getRandomByte());
          if(this._pattern.test(result))
          {
            return result;
          }
        }        
      }, this)
      .join('');  
  }    

};

Also if someone has suggestion to make it better or more secure, you are welcome.

One more thing, the lenght of genereated password is fixed value="16", perhaps that can be bad idea and wouldn't be better to generate password with random lenght, for ex. betwen 12 - 16, or something else.

Answer 1

Your problem is Math.random(). That is not cryptographically secure. I think it is securely seeded on most implementations, though, so if you are just generating one password you are fine. But if you use this to generate loads of passwords they will become predictable. That is bad. But it is sort of hard to work around in JavaScript - if you don't have a CSPRNG, you don't have a CSPRNG...

There might be other issues as well that I have missed, but that is what stands out to me.

As for password length, there are way fewer passwords that are 1 to 15 characters long than there are passwords that are 16 characters long, so you aren't really missing out on a lot. If you still do want a random length anyway, you need to think about what distribution you draw the length from. Using a uniform distribution (e.g. your _getRandomByte) will give you less entropy and not more, since you are to likely to generate short passwords.

Answer 2

The overall principle is valid: generate a password by selecting length characters at random. The details are where the problems lie:

1. You must use a cryptographically secure random number generator. Math.random() isn't so, and must never be used for this.
2. The way you're filtering the random choices is correct (all characters are selected with equal probability) but inefficient; you have a loop where you draw a random byte (256 possible values) and throw it away if it isn't one of your allowable characters, repeating until you pick a suitable one. The set of allowable characters is I guess about 95, which means most of the time you get a "miss." If you first take the remainder of dividing the random byte between 128 you also get correct results and you improve the "hit rate" noticeably. (This a is standard random number generation trick—you should use a library that does this for you already!)
3. You need to think clearly about the security target and thus the minimum allowable password length. If your passwords draw from 95 distinct characters, then each character contributes about log2(95) = 6.6 bits of entropy. You need 10 characters to get to 64 bits of entropy, which I think is the bare minimum anybody should ever use (website logins for sites where it wouldn't break the bank if somebody cracked the password, and you might want to memorize the passwords). 12 characters gets you to 79 bits, which I think is a good balance between length and security.

Answer 3

You should validate the length parameter. If you call Password.generate(), you may expect that a password is generated, but actually it returns an empty string.