mrcrgl / node-keygen

Random String generator for node.js
18 stars 1 forks source link

Not cryptographically random #1

Open markstos opened 9 years ago

markstos commented 9 years ago

Math.random() is not cryptographically random. It seems worth mentioning this in the README, or switching to a random number generator which is considered cryptographically random, such as crypto.randomBytes in Node.js.

mrcrgl commented 9 years ago

You're right @markstos. I'm actually quiet busy but I've added it to my todo list. If you like, feel free to create a pull request.

markstos commented 9 years ago

I'm not sure how to fix it, as the cryptographically secure API returns random bytes, not a random number:

https://nodejs.org/api/crypto.html#crypto_crypto_randombytes_size_callback

I guess you could do something like:

parseInt(crypto.randomBytes(16).toString('hex'),16);

The first "16" is the number of bytes, while the second "16" tells parseInt to convert from base 16.

The problem with this approach is I'm not sure what the lower and upper bounds are for the random numbers generated.

Also, Math.random() always succeeds, while in theory randomBytes() can run out of entropy and throw.

Personally, I don't need all the options, so I use the return value from this directly as the random string I need:

crypto.randomBytes(16).toString('hex')