Remove nsIEntropyCollector and its implementation

RESOLVED FIXED in Firefox 49

Status

()

Core
Security: PSM
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: briansmith, Assigned: keeler)

Tracking

(Blocks: 1 bug)

Trunk
mozilla49
Points:
---

Firefox Tracking Flags

(e10s-, firefox49 fixed)

Details

(Whiteboard: [psm-assigned])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 763387 [details] [diff] [review]
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.
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 2

5 years ago
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: --- → +
tracking-e10s: + → -
Attachment #763387 - Flags: superreview?(wtc)
(Assignee)

Updated

2 years ago
Status: ASSIGNED → NEW
Whiteboard: [psm-backlog]
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1263015
Created attachment 8751028 [details]
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/
Attachment #8751028 - Flags: review?(mounir)
Attachment #8751028 - Flags: review?(mgoodwin)
(Assignee)

Updated

2 years ago
Attachment #763387 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Assignee: nobody → dkeeler
(Assignee)

Updated

2 years ago
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.
(Assignee)

Updated

2 years ago
Attachment #8751028 - Flags: review?(mounir) → review?(mrbkap)

Updated

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

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8dd88e2a1976
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Comment 12

2 years ago
Created attachment 8755741 [details] [diff] [review]
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
Attachment #8755741 - Flags: checkin?

Updated

2 years ago
Attachment #8755741 - Flags: checkin? → checkin+
Whoops - thanks.
You need to log in before you can comment on or make changes to this bug.