Closed
Bug 80841
Opened 23 years ago
Closed 8 years ago
Mozilla should feed PSM/NSS with more entropy
Categories
(Core Graveyard :: Security: UI, defect, P3)
Tracking
(Not tracked)
RESOLVED
WORKSFORME
People
(Reporter: ddrinan0264, Unassigned)
References
Details
(Whiteboard: [kerh-ehz])
Attachments
(3 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
The current mozilla entropy implementation does not feed enough entropy to
PSM/NSS. Add more sources of entropy.
Comment 1•23 years ago
|
||
Comment 2•23 years ago
|
||
Placing a patch in this file that does the following:
1) Makes sure the variable gEntropyCollector in nsGlobalWindow.cpp gets a
non-NULL value.
2) Provides more entropy to the Random Number Generator.
Comment 3•23 years ago
|
||
Whoa, didn't the earlier discussions about the entropy collector (when it was
first added into nsGlobalWindow.cpp) state that updating the random numbers
could be quite expensive? In that case we *really* don't wanto update for every
mouse move, maybe we can compromize and update every 100th mouse move, or something?
I tested it on my 400Mhz PC and could not see any difference in my 'before' and
'after' tests.
Also, this is what Communicator has done for years, and there again there is no
performance problem.
Comment 5•23 years ago
|
||
asked jst for r=
Comment 6•23 years ago
|
||
Ok, maybe my memory is flaky here, if this is what 4.x did, then there shouldn't
be a problem. But do we really need to do this for mouse movement in chrome too
(which is extremely performance critical)? Can we leave the check for
!mChromeEventHandler in there?
Comment 7•23 years ago
|
||
hm, when I left it in, thet test always failed and the entropy was never
updated.
Comment 8•23 years ago
|
||
Are you saying entropy was *never* updated before this change then?
Comment 9•23 years ago
|
||
After the XPCDOM landing, entropy was never updated, because the first time
through the NSS component would fail to initialize because there was on profile
directory to initialize with.
Pre-XPCDOM, there was entropy added. My comment was about my patch when chaning
the event type to MOUSE_MOVE.
I'm in the middle of building my tree, will work on more patches once my build
finishes.
Comment 10•23 years ago
|
||
Regarding the cost of entropy collection, there are two separate
activities: collecting entropy, and using the collected entropy
to update the PRNG. Updating the PRNG can indeed be expensive.
But it is not necessary to update the PRNG every time new entropy
is collected.
PSM 1.x had an architecture that worked like this:
When an event occured that was a source of entropy, the entropy
information was collected in a buffer. This is very quick.
When some code needs to use the PRNG, it first uses any buffered
entropy to update the PRNG, then gets the new value from the
PRNG. So, PRNG updates occur only when it is necessary to use
the PRNG, not at every entropic event.
This strategy should continue to be used in PSM 2.x. Entropy
collection should be separate from PRNG updates. If entropy
collection has a measurable effect on performance, then it is
implemented incorrectly.
Comment 11•23 years ago
|
||
Ok, sounds reasonable, if this starts showing up on profiles we can re-address
the performance issues here.
Comment 12•23 years ago
|
||
In working, I realized that whenever the message is NS_EVENT_MOUSE_MOVE, there
is always a mChromeEventHandler there so I couldn't take out the test for
!mChromeEventHandler. Instead I've taken every 100th mouse move event.
Although I agree nelsonb's solution is the best solution, we (the PSM team)
doesn't have enough cycles to fully implement it before M0.91. So we'd like to
submit this for now and fully implement nelsonb's solution for M0.92
The reason we can't use PSM 1.x's solution is because it was dependent on the
control connection and since PSM 2 no longer has the concept of a control
connection, there is some design work that needs to be done to get the solution
put back into PSM 2.
Patch forthcoming.
Comment 13•23 years ago
|
||
Comment 14•23 years ago
|
||
I assume the patch is a cvs diff -wu patch since the indentation is incorrect,
if not then that needs to be fixed before checking in. Also, there's no reason
for gEntropyUpdateCnt to be a global static, it can be declared within the scope
where it's used and incremented to make the code a bit cleaner. Other than that,
sr=jst
Comment 15•23 years ago
|
||
Besides making that static function-scoped, how about renaming to ...Count?
Count is so much less cybercrude than Cnt, too.
/be
Comment 16•23 years ago
|
||
Comment 17•23 years ago
|
||
r=mcgreer
Comment 18•23 years ago
|
||
Above fix has been checked in. Will leave bug open for now until we get the
full implementation done.
Target Milestone: --- → 2.0
Updated•23 years ago
|
Keywords: nsenterprise
Updated•23 years ago
|
Priority: P3 → P2
Comment 21•23 years ago
|
||
P2
Comment 22•23 years ago
|
||
Moving to P3 we determined that we have enough entropy. More would be nice, but
this is a lot of work.
Priority: P2 → P3
Updated•23 years ago
|
Keywords: nsenterprise
Comment 24•23 years ago
|
||
Move to future. Won't have time to fix these for 2.1
Target Milestone: 2.1 → Future
Updated•23 years ago
|
QA Contact: ckritzer → junruh
Comment 25•22 years ago
|
||
See also bug 88847, which suggests a way to use all possible entropy without a
performance penalty.
Comment 26•21 years ago
|
||
Mass reassign Javi's old PSM bugs to nobody
Assignee: javi → nobody
QA Contact: junruh → nobody
Target Milestone: Future → ---
Updated•19 years ago
|
Whiteboard: [kerh-ehz]
Updated•18 years ago
|
QA Contact: nobody → ui
This is unnecessary at this point - we use strong(er) randomness provided by the OS.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
Assignee | ||
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•