-
Notifications
You must be signed in to change notification settings - Fork 3.4k
update to use cryptographic random if it is available #2447
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
FS.createDevice('/dev', 'urandom', function() { return Math.floor(Math.random()*256); }); | ||
var random_device; | ||
// for modern web browsers | ||
if ( typeof( crypto ) !== "undefined" ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the coding conventions in surrounding code. This is how it should look
if (typeof crypto !== 'undefined) {
..
} else if (typeof ..) {
..
}
Thanks for review. I fixed to coding rule and environment check method, and then I rebase/squash the commit. Please re-review and merge. |
var random_device; | ||
if (typeof crypto !== 'undefined') { | ||
// for modern web browsers | ||
random_device = function() { return crypto.getRandomValues(new Uint8Array(1))[0]; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One final thing, let's optimize this a little. Can create one such array before this line, and reuse it in the function. Typed array creation is notoriously slow, plus it creates garbage.
optimize cryptographic random generator.
I fixed to the optimization. And, I was measured it on Chromium-30, result is good.
|
Thanks! I think it's best to avoid using the |
update to use cryptographic random if it is available
I know this is old, but I've been going through this now. Given the broken state of JS cryptographic randomness I think that any reliance on Math.random() (even as a fallback) is a bad idea, especially seeing as how emscripten is very handy for converting cryptographic functions to JS. That said, I'm aware that some use-cases may need broader support or don't care about cryptographic randomness, so the Math.random() fallback is useful for them. Additionally, in order to support IE11, you need to check for window.msCrypto instead of window.crypto, as in this snippet used in openpgpjs:
May I suggest using cryptographically secure randomness by default, with an option to allow a Math.Random() fallback if cryptographic randomness is not required? |
Forgot to add that I can (probably) write that and submit a PR, unless someone else who is more familiar with emscripten's architecture wants to give it a go:) |
PR for the ms fix is welcome of course. Not sure where specifically you are suggesting we use secure crypto that we aren't now? |
@kripken this bit -
References:
Or as Mozilla says on their Math.random() page -
|
That code is hit only if |
@kripken at the risk of unnecessarily repeating myself, I'll copy-and-paste from my original message:
In other words, by default Math.random() should not be allowed at all, under any circumstances. An option can be set that allows for a Math.random() fallback if cryptographically insecure randomness is desirable (e.g. when rendering game objects where speed is more important). |
Sorry if I'm being dense here. But I am having trouble understanding what the suggestion actually means. Let's say we don't allow Math.random() - what would execute instead of it, if the browser doesn't support |
@kripken it should abort with an error. There are some alternative JS libraries that try provide a cryptographically secure PRNG by getting entropy from mouse movement etc., but you don't really want a warm-up time of 30+ seconds as a fallback, plus a lot of them are poorly maintained or not very well written (so they're not ideal). In other words, the preferred outcome should be:
|
Got it. This is just for I'm not opposed to making this change to the default, but at minimum we should do it with a clear error message pointing to an optional flag to disable the assert, so people can override it. Also, I'd like to hear what other people think about this, in particular @juj . |
Yes, just for /dev/random, all random functions hinge off there anyway. If there's not enough visibility here I can move it to an issue instead? |
Reading http://man7.org/linux/man-pages/man4/random.4.html , I understand that In that respect, I'd prefer that our implementation of those should be fully cryptographically secure by default. If we are in a scenario where we can't provide that, my thinking is that those files would not exist in the filesystem at all (or reading would generate error or eof, whichever would feel most idiomatic to signal "not available"). Then a build flag e.g. For any other APIs which do not intend to guarantee crypto security (C |
@juj cryptographers are careful to never guarantee security:-P I agree with your conclusion - what is the knock-on effect for an app expecting /dev/random and it suddenly isn't there? Would the app be expected to handle that? |
The pull-request based on #2439 (comment).
References:
Result of
python tests/runner.py test_random_device
And, I tested Firefox-30 and Chromium-35 on Ubuntu-14.04 that is ok.
( The test is modify the code simply, add a logging in the code path and call std::random() and read std::ifstream("/dev/[u]random"). )