Closed Bug 883718 Opened 6 years ago Closed 3 years ago

Remove nsIEntropyCollector and its implementation

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
e10s - ---
firefox49 --- fixed

People

(Reporter: briansmith, Assigned: keeler)

References

(Blocks 1 open bug)

Details

(Whiteboard: [psm-assigned])

Attachments

(2 files, 1 obsolete file)

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.
Attachment #763387 - Flags: superreview?(wtc)
Attachment #763387 - Flags: review?(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.
Attachment #763387 - Flags: review?(jst) → review+
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.
Assignee: brian → nobody
Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".
tracking-e10s: --- → +
Attachment #763387 - Flags: superreview?(wtc)
Status: ASSIGNED → NEW
Whiteboard: [psm-backlog]
Duplicate of this bug: 1263015
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/
Attachment #8751028 - Flags: review?(mounir)
Attachment #8751028 - Flags: review?(mgoodwin)
Attachment #763387 - Attachment is obsolete: true
Assignee: nobody → dkeeler
Whiteboard: [psm-backlog] → [psm-assigned]
I see there is a new fancy review board :) r+ though I'm not sure I'm the right person to review this :)
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!
Attachment #8751028 - Flags: review?(mgoodwin) → review+
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.
Attachment #8751028 - Flags: review?(mounir) → review?(mrbkap)
Attachment #8751028 - Flags: review?(mrbkap) → review+
Comment on attachment 8751028 [details]
MozReview Request: bug 883718 - remove nsIEntropyCollector and implementation r?mounir r?mgoodwin

https://reviewboard.mozilla.org/r/51769/#review48895
https://hg.mozilla.org/mozilla-central/rev/8dd88e2a1976
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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
Attachment #8755741 - Flags: checkin?
Attachment #8755741 - Flags: checkin? → checkin+
You need to log in before you can comment on or make changes to this bug.