Mozilla should feed PSM/NSS with more entropy

RESOLVED WORKSFORME

Status

P3
critical
RESOLVED WORKSFORME
18 years ago
2 years ago

People

(Reporter: ddrinan0264, Unassigned)

Tracking

1.0 Branch
x86
Windows NT
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [kerh-ehz])

Attachments

(3 attachments)

(Reporter)

Description

18 years ago
The current mozilla entropy implementation does not feed enough entropy to 
PSM/NSS. Add more sources of entropy.

Comment 1

18 years ago
Created attachment 34668 [details] [diff] [review]
Patch that always creates a nsIEntropyCollector and provids more entropy

Comment 2

18 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.
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?

Comment 4

18 years ago
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

18 years ago
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?

Comment 7

18 years ago
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?

Comment 9

18 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.
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.

Comment 12

18 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

18 years ago
Created attachment 34828 [details] [diff] [review]
Patch that uses every 100th mouse move for entropy.
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

Comment 16

18 years ago
Created attachment 35025 [details] [diff] [review]
Newer patch w/ suggestions incorporated.

Comment 17

18 years ago
r=mcgreer

Comment 18

18 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
(Reporter)

Comment 19

18 years ago
->p3
Severity: normal → critical
Priority: -- → P3

Updated

18 years ago
Blocks: 82941

Comment 20

18 years ago
Mass reassigning target to 2.1
Target Milestone: 2.0 → 2.1

Updated

18 years ago
Keywords: nsenterprise

Updated

18 years ago
Priority: P3 → P2

Comment 21

18 years ago
P2

Comment 22

18 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

18 years ago
Keywords: nsenterprise

Comment 23

18 years ago
Mass assigning QA to ckritzer.
QA Contact: junruh → ckritzer

Comment 24

17 years ago
Move to future. Won't have time to fix these for 2.1
Target Milestone: 2.1 → Future

Updated

17 years ago
QA Contact: ckritzer → junruh

Comment 25

17 years ago
See also bug 88847, which suggests a way to use all possible entropy without a
performance penalty.

Updated

17 years ago
Blocks: 157812
Mass reassign Javi's old PSM bugs to nobody
Assignee: javi → nobody
QA Contact: junruh → nobody
Target Milestone: Future → ---

Updated

14 years ago
Component: Security: UI → Security: UI
Product: PSM → Core

Updated

13 years ago
Whiteboard: [kerh-ehz]
QA Contact: nobody → ui

Updated

10 years ago
Version: psm2.0 → 1.0 Branch
This is unnecessary at this point - we use strong(er) randomness provided by the OS.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
(Assignee)

Updated

2 years ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.