Multiple security vulnerabilities in "rando" and "randoSequence" functions.

[zfi2]

The current implementation of the rando and randoSequence functions is not cryptographically secure and should not be used for any security-sensitive purpose. It contains several vulnerabilities that introduce bias and rely on insecure fallbacks.

1. Insecure fallback to Math.random()

• Issue: The cryptoRandom function includes a try...catch block that defaults to using Math.random() if window.crypto.getRandomValues() is unavailable or fails.
• Impact: Math.random() is not a cryptographically secure pseudo-random number generator (CSPRNG). Its output is predictable and should not be used for security applications like generating tokens, keys, or for any form of authentication. A secure generator must not have an insecure fallback; it should fail loudly if a secure source of randomness is not available.

2. Biased filtering of random values

• Issue: The code filters the output of crypto.getRandomValues with the condition cryptoRandoms[i] < 4000000000.
• Impact: A Uint32Array can contain values up to 4,294,967,295. By discarding the top ~7% of possible values, the code introduces a significant bias into the "random" numbers it produces. This non-uniform distribution weakens the randomness and can make the output more predictable.

3. Introduction of modulo bias

• Issue: To generate a number within a specific range, the code scales the random value using multiplication and addition (e.g., cryptoRandom() * (arg2 - arg1 + 1) + arg1).
• Impact: This method leads to modulo bias. When mapping a large range of random numbers to a smaller one, some results in the smaller range will appear more frequently than others if the size of the large range is not a perfect multiple of the smaller one. This makes the output distribution uneven, which is a critical flaw in a random number generator. The correct approach is to use a technique like rejection sampling.

4. Unnecessary and obscure string manipulations

• Issue: The cryptoRandom function converts the secure random integers from getRandomValues into strings, slices them, joins them, and converts them back to a number.
• Impact: This implementation is unnecessarily complex, inefficient, and difficult to audit for security. It obscures the underlying random data and provides no cryptographic benefit. Standard practice is to work directly with the integer values produced by the CSPRNG. This convoluted logic could also introduce unforeseen bugs or vulnerabilities.

Recommendation

The rando and randoSequence functions should be completely rewritten to address these vulnerabilities. A secure implementation must:

1. Exclusively use window.crypto.getRandomValues().
2. Throw an error if the Crypto API is unavailable, rather than falling back to an insecure method.
3. Use rejection sampling to avoid modulo bias when generating numbers within a specific range.
4. Operate on integer values directly, avoiding insecure and complex string manipulations.

[rfl890]

I've opened a PR to fix the string manipulation part, but it seems the author has abandoned the project. Oh well.

[TapioOfTapiola]

Fallback to Math.random, not using the whole 2**32 integers - sigh. Sad. Even as someone who doesn't live from coding I follow better (best) practices.