Closed
Bug 1228571
Opened 9 years ago
Closed 9 years ago
random_generateSeed leaves seed uninitialized when opening /dev/urandom fails
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox43 | --- | unaffected |
firefox44 | --- | fixed |
firefox45 | --- | fixed |
b2g-v2.5 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
(Keywords: sec-other)
Attachments
(1 file)
1.33 KB,
patch
|
cpeterson
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
If we can't open /dev/urandom, we used to have a fallback path that used PRMJ_Now(). Bug 1206596 refactored this function and now we do nothing if we can't open /dev/urandom. This only affects Linux (on Windows, OS X, Android and the BSDs we don't use /dev/urandom but rand_s or arc4random). Filing this s-s just to be sure. In theory Math.random() should not be used near anything security-related but who knows.
Comment 1•9 years ago
|
||
I don't think we're really making any guarantees about Math.random() so I'll just mark it sec-other. It could be unhidden, even.
Keywords: sec-other
Assignee | ||
Comment 2•9 years ago
|
||
Use PRMJ_Now() if we couldn't read from /dev/urandom. This matches what we did before.
Attachment #8696510 -
Flags: review?(cpeterson)
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #1) > I don't think we're really making any guarantees about Math.random() so I'll > just mark it sec-other. It could be unhidden, even. Yeah I guess I was overly cautious.
Group: javascript-core-security
status-firefox43:
--- → unaffected
status-firefox44:
--- → affected
status-firefox45:
--- → affected
Comment 4•9 years ago
|
||
Comment on attachment 8696510 [details] [diff] [review] Patch Review of attachment 8696510 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. I was literally hacking on this issue today as part of a fix for bug 1167248, a Windows crash in this function. :) (In reply to Andrew McCreight [:mccr8] from comment #1) > I don't think we're really making any guarantees about Math.random() so I'll > just mark it sec-other. True, though this C++ function is used for more than just Math.random(). In particular, it is also used to seed ExecutableAllocator::computeRandomAllocationAddress() for JIT VirtualAllocs on Windows. But making this bug public is OK.
Attachment #8696510 -
Flags: review?(cpeterson) → review+
Assignee | ||
Comment 6•9 years ago
|
||
Comment on attachment 8696510 [details] [diff] [review] Patch Approval Request Comment [Feature/regressing bug #]: Bug 1206596. [User impact if declined]: Bogus Math.random() results if /dev/urandom is not available somehow. Broken websites. [Describe test coverage new/current, TreeHerder]: NA [Risks and why]: Very low risk. [String/UUID change made/needed]: None.
Attachment #8696510 -
Flags: approval-mozilla-aurora?
Comment 7•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6d8e9838e366
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 8•8 years ago
|
||
Comment on attachment 8696510 [details] [diff] [review] Patch Sure, let's take that in 45.
Attachment #8696510 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #8) > Sure, let's take that in 45. Well when I requested approval 2 weeks ago Aurora was still 44, so it was meant for that release :)
Flags: needinfo?(sledru)
Comment 10•8 years ago
|
||
Comment on attachment 8696510 [details] [diff] [review] Patch [Triage Comment] You mean beta I guess:)
Flags: needinfo?(sledru)
Attachment #8696510 -
Flags: approval-mozilla-aurora+ → approval-mozilla-beta+
This doesn't apply cleanly to beta. Can we get a rebased patch if this needs to land there?
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Wes Kocher (:KWierso) (On vacation until Dec 28) from comment #11) > This doesn't apply cleanly to beta. Can we get a rebased patch if this needs > to land there? Pushed it: https://hg.mozilla.org/releases/mozilla-beta/rev/7bd6d6fd32da
Flags: needinfo?(jdemooij)
Comment 13•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/7bd6d6fd32da
status-b2g-v2.5:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•