883718 - Remove nsIEntropyCollector and its implementation

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: remove nsIEntropyCollector and its implementation

nsIEntropyCollector seems unnecessary given the current implementation of NSS on modern operating systems. The idea of it is to mix additional entropy from system events. The current implementation in Gecko uses mouse movement events to add entropy. Obviously, this only works on systems that have a mounse--i.e. not mobile platforms. We're already relying on the OS and NSS to give us good randomness without mouse movement events on mobile, those events must not be needed for desktop either.

Less obviously, if one wanted to build a content/chrome-separated browser in Gecko with PSM (only) in the chrome process, then this communication channel would be an unnecessarily burdensome complication to deal with.

This would also simplify some DOM and PSM code to be friendlier to newcomers.

I did not measure whether there is any positive performance impact with this change, with respect to mouse movement tracking performance.

Counter-arguments about why this change is not a good idea are also appreciated.

Comment 1

[Johnny Stenback (:jst)]

Comment on attachment 763387 [details] [diff] [review]
remove nsIEntropyCollector and its implementation

The removal of security/manager/ssl/src/PSMContentDownloader.h looks unrelated to this change. Also, that file is not in m-c, it seems.

r=jst for the rest.

Comment 2

[Wan-Teh Chang]

Comment on attachment 763387 [details] [diff] [review]
remove nsIEntropyCollector and its implementation

I think we need to replace this code with periodic calls to
PK11_RandomUpdate() with random bytes from /dev/urandom or
RtlGenRandom.

Comment 3

[Chris Peterson [:cpeterson]]

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

Comment 5

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

Attachment: MozReview Request: bug 883718 - remove nsIEntropyCollector and implementation r?mounir r?mgoodwin

The rationale behind nsIEntropyCollector was to supplement NSS' source of
entropy with randomness from mouse move events. This obviously doesn't work on
platforms without a mouse (e.g. mobile platforms). Furthermore, as NSS seeds its
random number generator with robust randomness from the operating system, this
is unnecessary anyway. The primary concern is that initialization of the random
number generator must happen after forking, which is exactly what we do with the
child process in e10s mode.

Review commit: https://reviewboard.mozilla.org/r/51769/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51769/

Comment 6

[Mounir Lamouri (:mounir)]

I see there is a new fancy review board :) r+ though I'm not sure I'm the right person to review this :)

Comment 7

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8751028 [details]
MozReview Request: bug 883718 - remove nsIEntropyCollector and implementation r?mounir r?mgoodwin

https://reviewboard.mozilla.org/r/51769/#review48779

Looks good to me!

Comment 8

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

Thanks for the reviews. Mounir - I chose you as a reviewer based on a quick hg annotate of dom/base/nsGlobalWindow.cpp, but it looks like the changes you made there weren't really specific to the entropy collection. Maybe it would be best if I just redirect the review.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8751028 [details]
MozReview Request: bug 883718 - remove nsIEntropyCollector and implementation r?mounir r?mgoodwin

https://reviewboard.mozilla.org/r/51769/#review48895

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dd88e2a1976

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8dd88e2a1976

Comment 12

[:Cykesiopka]

Attachment: Followup: Remove nsIBufEntropyCollector.idl

nsIBufEntropyCollector.idl is no longer used post https://hg.mozilla.org/mozilla-central/rev/8dd88e2a1976, but was not removed. This (trivial) patch just removes the file.

This is NPOTB, but here's a try push anyways:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d58a4ede1eea

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5677ef82f274

Comment 14

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

Whoops - thanks.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5677ef82f274