Closed
Bug 1220223
Opened 9 years ago
Closed 9 years ago
don't load PKCS11 modules in safe mode
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: keeler, Assigned: keeler)
References
Details
Attachments
(1 file)
Buggy PKCS11 modules can cause problems for Firefox (e.g. crashes - this may be what's going on in bug 1164826). If started in safe mode, Firefox shouldn't load these modules.
Updated•9 years ago
|
Assignee: nobody → dkeeler
Assignee | ||
Comment 1•9 years ago
|
||
bug 1220223 - don't load PKCS11 modules in safe mode
Attachment #8682821 -
Flags: review?(mgoodwin)
Attachment #8682821 -
Flags: review?(benjamin)
Assignee | ||
Comment 2•9 years ago
|
||
:bsmedberg - the review request is mainly to make sure I did the inSafeMode check correctly and that there aren't any gotchas there. Thanks!
Updated•9 years ago
|
Attachment #8682821 -
Flags: review?(mgoodwin) → review+
Comment 3•9 years ago
|
||
Comment on attachment 8682821 [details] MozReview Request: bug 1220223 - don't load PKCS11 modules in safe mode https://reviewboard.mozilla.org/r/24157/#review21651 Looks good. ::: security/manager/ssl/tests/unit/test_pkcs11_safe_mode.js:14 (Diff revision 1) > +// Registers an nsIXULRuntime so the test can control whether or not the Is there a reason you don't want this to be an nsIXULAppInfo? If not, there's at least one other place (test_cert_blocklist.js) where we use an nsIXULAppInfo (which implements nsIXULRuntime) and there will likely be others in the future. Could we create registerAppInfo in head_psm.js and use that instead? Obviously no issue if not.
Comment 4•9 years ago
|
||
Comment on attachment 8682821 [details] MozReview Request: bug 1220223 - don't load PKCS11 modules in safe mode https://reviewboard.mozilla.org/r/24157/#review21707 ::: security/manager/ssl/nsNSSComponent.cpp:1027 (Diff revision 1) > + if (!runtime) { I don't think this check is "safe". XUL apps have a nsIXULRuntime, but other contexts (xpcshell certainly, probably some other unit tests, and some weird embedded situations) aren't going to have one. This should probably fail-closed (don't load external PKCS modules) but shouldn't fail completely.
Attachment #8682821 -
Flags: review?(benjamin)
Updated•9 years ago
|
Attachment #8682821 -
Flags: review-
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8682821 [details] MozReview Request: bug 1220223 - don't load PKCS11 modules in safe mode Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24157/diff/1-2/
Attachment #8682821 -
Flags: review- → review?(benjamin)
Assignee | ||
Comment 6•9 years ago
|
||
Hmmm. reviewboard seems to have eaten my replies. I'll reproduce them here: (In reply to Mark Goodwin [:mgoodwin] from comment #3) > Comment on attachment 8682821 [details] > MozReview Request: bug 1220223 - don't load PKCS11 modules in safe mode > > https://reviewboard.mozilla.org/r/24157/#review21651 > > Looks good. > > ::: security/manager/ssl/tests/unit/test_pkcs11_safe_mode.js:14 > (Diff revision 1) > > +// Registers an nsIXULRuntime so the test can control whether or not the > > Is there a reason you don't want this to be an nsIXULAppInfo? If not, > there's at least one other place (test_cert_blocklist.js) where we use an > nsIXULAppInfo (which implements nsIXULRuntime) and there will likely be > others in the future. Could we create registerAppInfo in head_psm.js and use > that instead? > > Obviously no issue if not. That's what I tried originally. This resulted in a mismatch due to the nsIXULAppInfo being registered under "@mozilla.org/xre/app-info;1", whereas nsNSSComponent gets an nsIXULRuntime from "@mozilla.org/xre/runtime;1". I could change the latter, but while it does appear that some nsIXULRuntime instances are obtained from "app-info", most are from "runtime", so I thought it good to be as consistent as possible there. (In reply to Benjamin Smedberg [:bsmedberg] from comment #4) > Comment on attachment 8682821 [details] > MozReview Request: bug 1220223 - don't load PKCS11 modules in safe mode > > https://reviewboard.mozilla.org/r/24157/#review21707 > > ::: security/manager/ssl/nsNSSComponent.cpp:1027 > (Diff revision 1) > > + if (!runtime) { > > I don't think this check is "safe". XUL apps have a nsIXULRuntime, but other > contexts (xpcshell certainly, probably some other unit tests, and some weird > embedded situations) aren't going to have one. This should probably > fail-closed (don't load external PKCS modules) but shouldn't fail completely. OK - sounds like a plan.
Comment 7•9 years ago
|
||
Comment on attachment 8682821 [details] MozReview Request: bug 1220223 - don't load PKCS11 modules in safe mode https://reviewboard.mozilla.org/r/24157/#review22075
Attachment #8682821 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 8•9 years ago
|
||
Thanks for the reviews. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=535ece866038
Comment 10•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a48673c8ee79
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•