The fallback to using Math.random() is clearly noted to be used only for testing purposes, but its inclusion in the module itself instead of as a test stub/override within the test suite might make it unnecessarily a target for various script injection, XSS, malicious extensions, rogue dependencies, etc.
An attacker that manages to manipulate the global scope may be able to trick this into using Math.random(), which would lead to weak keys being generated.
Fully acknowledging that an attacker with script injection capabilities could very well do more damage than compromise the entropy source, I think moving this use of Math.random() out of a conditional path within the module itself into a test stub would be a good practice. Doing so would at least allow application developers to Object.freeze(window.crypto) and expect that tampering with global variables would lead to an exception instead of possible silent compromise of entropy source for keys.
https://github.com/bcoin-org/bcrypto/blob/master/lib/js/random.js#L157-L167
The fallback to using
Math.random()is clearly noted to be used only for testing purposes, but its inclusion in the module itself instead of as a test stub/override within the test suite might make it unnecessarily a target for various script injection, XSS, malicious extensions, rogue dependencies, etc.An attacker that manages to manipulate the global scope may be able to trick this into using
Math.random(), which would lead to weak keys being generated.Fully acknowledging that an attacker with script injection capabilities could very well do more damage than compromise the entropy source, I think moving this use of
Math.random()out of a conditional path within the module itself into a test stub would be a good practice. Doing so would at least allow application developers toObject.freeze(window.crypto)and expect that tampering with global variables would lead to an exception instead of possible silent compromise of entropy source for keys.:key: :sparkles: :two_hearts: