80841 - Mozilla should feed PSM/NSS with more entropy

Status: RESOLVED WORKSFORME

(Core Graveyard :: Security: UI, defect, P3)

Description

[David P. Drinan]

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

Comment 1

[Javier Delgadillo]

Attachment: Patch that always creates a nsIEntropyCollector and provids more entropy

Comment 2

[Javier Delgadillo]

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

[Johnny Stenback (:jst)]

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

[Bob Lord]

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

[Javier Delgadillo]

asked jst for r=

Comment 6

[Johnny Stenback (:jst)]

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

[Javier Delgadillo]

hm, when I left it in, thet test always failed and the entropy was never 
updated.

Comment 8

[Johnny Stenback (:jst)]

Are you saying entropy was *never* updated before this change then?

Comment 9

[Javier Delgadillo]

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

[Nelson Bolyard (seldom reads bugmail)]

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

[Johnny Stenback (:jst)]

Ok, sounds reasonable, if this starts showing up on profiles we can re-address
the performance issues here.

Comment 12

[Javier Delgadillo]

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

[Javier Delgadillo]

Attachment: Patch that uses every 100th mouse move for entropy.

Comment 14

[Johnny Stenback (:jst)]

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

[Brendan Eich [:brendan]]

Besides making that static function-scoped, how about renaming to ...Count? 
Count is so much less cybercrude than Cnt, too.

/be

Comment 16

[Javier Delgadillo]

Attachment: Newer patch w/ suggestions incorporated.

Comment 17

[Ian McGreer]

r=mcgreer

Comment 18

[Javier Delgadillo]

Above fix has been checked in.  Will leave bug open for now until we get the 
full implementation done.

Comment 19

[David P. Drinan]

->p3

Comment 20

[Stephane Saux]

Mass reassigning target to 2.1

Comment 21

[Stephane Saux]

P2

Comment 22

[Stephane Saux]

Moving to P3 we determined that we have enough entropy. More would be nice, but 
this is a lot of work.

Comment 23

[John Unruh]

Mass assigning QA to ckritzer.

Comment 24

[Stephane Saux]

Move to future. Won't have time to fix these for 2.1

Comment 25

[Kai Engert (:KaiE:)]

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

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Mass reassign Javi's old PSM bugs to nobody

Comment 27

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

This is unnecessary at this point - we use strong(er) randomness provided by the OS.