sodiray / radash

Functional utility library - modern, simple, typed, powerful
https://radash-docs.vercel.app
MIT License
4.31k stars 173 forks source link

Improve random to cryptographically secure random numbers #32

Open kyoungduck opened 2 years ago

kyoungduck commented 2 years ago

Math.random is not safe at something related to security.(https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/random)

Actually this problem has not only security. When I make simple web game(https://peter-roulette.pages.dev/?data=2tjDqtNPAu4Rh) the random is biased.

So it can use https://developer.mozilla.org/ko/docs/Web/API/Crypto/getRandomValues

kyoungduck commented 2 years ago

Crypto.getRandomValues() supports NodeJS and web browser so we can use it.

image
sodiray commented 2 years ago

Hey @kyoungduck thanks for bringing this up. I can see the benefit if you're using functions from radash like shuffle, draw or random for a game.

Are there any downsides to consider?

SplittyDev commented 2 years ago

Since there is no minimum degree of entropy requirement, this has no downsides and does not block.

sodiray commented 2 years ago

Since there is no minimum degree of entropy requirement, this has no downsides and does not block.

Tell me this again like I'm just a self-taught web developer who didn't go to college and isn't a mathematician or security engineer 😅

A bit of a joke, but mostly serious, are you saying making this change isn't necessary or that it won't break anything if changed?

kyoungduck commented 2 years ago

logically there's no downsides.

But performance can be difference. I suggest this because Math.random is incredibly fast and Crypto.getRandomValues is not that slow as developer(who use this library) recognize performance issue.

Math.random x 155,374,747 ops/sec ±0.31% (101 runs sampled)
Crypto.getRandomValues x 913,723 ops/sec ±2.64% (73 runs sampled)

I test at Node v16.15.1 code: https://github.com/kyoungduck/random-benchmark

It's trade off but I think choose Crypto.getRandomValues is better. Because when you can see Math.random normal distribution here, it's very biased. I think this bias has an effect on developers developing business logic, and I actually did.

sodiray commented 2 years ago

I've been doing some reading up on this. I'm a fan. We could do something like this get-random-values package does to find the appropriate api to use. Instead of throwing an error if non exist we can fall back to Math.random.

SplittyDev commented 2 years ago

Since there is no minimum degree of entropy requirement, this has no downsides and does not block.

Tell me this again like I'm just a self-taught web developer who didn't go to college and isn't a mathematician or security engineer 😅

A bit of a joke, but mostly serious, are you saying making this change isn't necessary or that it won't break anything if changed?

Ahh sorry, what I meant is that usually secure random implementations wouldn't be perfect for this because they have to collect enough entropy from an entropy source to actually be secure, and usually they block if entropy is not enough, which could cause lag spikes. Since the secure random implementation suggested here doesn't have a minimum entropy requirement this is actually an advantage in this case, since it won't block while waiting for more entropy.

So my argument is for doing it and I don't see any obvious downsides.

UnKnoWn-Consortium commented 1 year ago

I tried to implement this a while ago. But it has more intricacies than one may think as the client (browser) and server (specifically NodeJS) APIs are very different. @rayepps May I know what is your take on JavaScript isomorphism, external dependencies, and build tools?