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)

1.0 Branch
x86
Windows NT
defect

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: ddrinan0264, Unassigned)

References

Details

(Whiteboard: [kerh-ehz])

Attachments

(3 files)

The current mozilla entropy implementation does not feed enough entropy to PSM/NSS. Add more sources of entropy.
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.
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.
asked jst for r=
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?
hm, when I left it in, thet test always failed and the entropy was never updated.
Are you saying entropy was *never* updated before this change then?
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.
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.
Ok, sounds reasonable, if this starts showing up on profiles we can re-address the performance issues here.
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.
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
Besides making that static function-scoped, how about renaming to ...Count? Count is so much less cybercrude than Cnt, too. /be
r=mcgreer
Above fix has been checked in. Will leave bug open for now until we get the full implementation done.
Target Milestone: --- → 2.0
->p3
Severity: normal → critical
Priority: -- → P3
Blocks: 82941
Mass reassigning target to 2.1
Target Milestone: 2.0 → 2.1
Keywords: nsenterprise
Priority: P3 → P2
P2
Moving to P3 we determined that we have enough entropy. More would be nice, but this is a lot of work.
Priority: P2 → P3
Keywords: nsenterprise
Mass assigning QA to ckritzer.
QA Contact: junruh → ckritzer
Move to future. Won't have time to fix these for 2.1
Target Milestone: 2.1 → Future
QA Contact: ckritzer → junruh
See also bug 88847, which suggests a way to use all possible entropy without a performance penalty.
Blocks: entropy
Mass reassign Javi's old PSM bugs to nobody
Assignee: javi → nobody
QA Contact: junruh → nobody
Target Milestone: Future → ---
Product: PSM → Core
Whiteboard: [kerh-ehz]
QA Contact: nobody → ui
Version: psm2.0 → 1.0 Branch
This is unnecessary at this point - we use strong(er) randomness provided by the OS.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: