1220223 - don't load PKCS11 modules in safe mode

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

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.

Comment 1

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

Attachment: MozReview Request: bug 1220223 - don't load PKCS11 modules in safe mode

bug 1220223 - don't load PKCS11 modules in safe mode

Comment 2

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

:bsmedberg - the review request is mainly to make sure I did the inSafeMode check correctly and that there aren't any gotchas there. Thanks!

Comment 3

[Mark Goodwin [:mgoodwin]]

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

[Benjamin Smedberg]

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.

Comment 5

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

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/

Comment 6

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

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

[Benjamin Smedberg]

Comment on attachment 8682821 [details]
MozReview Request: bug 1220223 - don't load PKCS11 modules in safe mode

https://reviewboard.mozilla.org/r/24157/#review22075

Comment 8

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

Thanks for the reviews. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=535ece866038

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a48673c8ee79

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a48673c8ee79