Closed Bug 868860 Opened 7 years ago Closed 7 years ago
.random() PRNG with a better seed
In bug 322529 comment 87, bsmith suggested opening a new bug to discuss improving Math.random()'s seed. This patch implements something similar to daw's "Level 2" suggestion from bug 322529 comment 74: seed Math.random() from something secure without changing the current PRNG algorithm. This patch seeds Math.random() from /dev/urandom (on XP_UNIX, including OSX) or rand_s() (on Windows). Seeding is done lazily so compartments that never call Math.random() have no overhead. JSRuntime no longer needs rngNonce because each compartment should be getting a very random seed, even for compartments that are seeded at the same PRMJ_Now() time. I investigated calling NSS' PK11_GenerateRandomOnSlot(), but that approach seemed messier. Firefox initializes SpiderMonkey before NSS, so JSRuntime would need to call NSS_Init() (and I don't know what side effects initializing NSS earlier might introduce). Firefox also unloads NSS before SpiderMonkey, so an "xpcom-shutdown" observer could call Math.random() and NSS would be gone.
Attachment #745699 - Flags: review?(luke)
I think it might be more appropriate to use CryptGenRandom for the seed on Windows. The rand_s documentation doesn't give me the warm fuzzies regarding its cryptographic soundness.
(In reply to Zack Weinberg (:zwol) from comment #1) > I think it might be more appropriate to use CryptGenRandom for the seed on > Windows. The rand_s documentation doesn't give me the warm fuzzies > regarding its cryptographic soundness. MSDN says rand_s() "depends on" RtlGenRandom(). And according to Wikipedia and other sources , CryptGenRandom() itself is implemented using RtlGenRandom() (plus the CryptoAPI overhead for CryptAcquireContext()).  http://cryptodox.com/CryptGenRandom
(cc'ing some other people that may know more about the history of Math.random) Could you explain a bit more about the security concern and problem being solved here?
Bug 322529 has a discussion of some concerns about Math.random(), including web content unwisely using Math.random() for JS crypto; side channel attacks from other compartments on the predictable seed; and the seed leaking addresses of heap or stack memory. Also, window.crypto.getRandomValues() is not available to workers. A more radical proposal from bug 322529 was to replace Math.random()'s PRNG algorithm, not just the seed, with output from window.crypto.getRandomValues().
(In reply to Luke Wagner [:luke] from comment #3) > Could you explain a bit more about the security concern and problem being > solved here? *For this bug*, the principal concern being addressed is cross-compartment information leakage via the random seeds not being different enough, AIUI. As Chris says, see bug 322529 for more. (In reply to Chris Peterson (:cpeterson) from comment #2) > MSDN says rand_s() "depends on" RtlGenRandom(). And according to Wikipedia > and other sources , CryptGenRandom() itself is implemented using > RtlGenRandom() (plus the CryptoAPI overhead for CryptAcquireContext()). Oh, that's unfortunate; it was RtlGenRandom itself that was not giving me the warm fuzzies. Still, that does mean there's no reason not to go for the simpler rand_s interface.
(In reply to Zack Weinberg (:zwol) from comment #5) > Oh, that's unfortunate; it was RtlGenRandom itself that was not giving me > the warm fuzzies. Related: bug 504270, and specifically bug 504270 comment 6.
Let me clarify that it is the *cryptographic soundness* of RtlGenRandom that is not giving me the warm fuzzies. Specifically, the description here of its internal algorithm: https://blogs.msdn.com/b/michael_howard/archive/2005/01/14/353379.aspx?Redirected=true#353493 gives me the impression that on any single machine, nearly all the intra-machine variation is coming from various cycle counters, and so it may not actually be equivalent to /dev/urandom in terms of "true" randomness produced. Having said that, I only objected because I had been under the impression that CryptGenRandom was a stronger RNG. If this is the best one can get right now on Windows, then that's going to have to do.
Attachment #745699 - Flags: review?(zackw)
Comment on attachment 745699 [details] [diff] [review] random-seed.patch The platform-specific gunk looks good to me.
Attachment #745699 - Flags: review?(zackw) → review+
Thanks for the thoughtful consideration. The option you mentioned of leaving the #define at the top of jsmath.cpp sounds like the right thing to do. If we needed this a second place, I'd probably want to put this in jswin.h (our Windows fudge-file).
Fix bustage: XP_UNIX must #include unistd.h to get read() and close() definitions. https://hg.mozilla.org/integration/mozilla-inbound/rev/ac913f0cde8e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.