1230377 - nsKeyObject doesn't implement nsNSSShutDownObject and as a result can hold alive NSS resources at shutdown

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Run `NSPR_LOG_MODULES=pipnss:5 ./mach xpcshell-test services/fxaccounts/tests/xpcshell/test_credentials.js`. The string "NSS SHUTDOWN FAILURE" will be in the output. This is due to nsKeyObject not observing NSS shutdown and releasing all of its NSS resources.

While looking into this, it seems that nsKeyObject was implemented with both symmetric and asymmetric keys in mind, but the asymmetric key part was never completed. We've gone this far without needing that extra capability, so we may as well remove it while we're here.

Comment 1

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

Attachment: MozReview Request: bug 1230377 - part 1/2: ensure nsKeyObject releases NSS resources on shutdown

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

Comment 2

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

Attachment: MozReview Request: bug 1230377 - part 2/2: simplify nsIKeyObject and nsIKeyObjectFactory

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

Comment 3

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8702742 [details]
MozReview Request: bug 1230377 - part 1/2: ensure nsKeyObject releases NSS resources on shutdown

https://reviewboard.mozilla.org/r/27807/#review26233

This looks perfectly clear, and it doesn't look like you missed anything. r=jcjones

Comment 4

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8702743 [details]
MozReview Request: bug 1230377 - part 2/2: simplify nsIKeyObject and nsIKeyObjectFactory

https://reviewboard.mozilla.org/r/27809/#review26237

Looks good here. Two questions, but they're more for me. r=jcjones

::: security/manager/ssl/nsKeyModule.cpp:115
(Diff revision 1)
> -  CK_MECHANISM_TYPE cipherMech;
> -  CK_ATTRIBUTE_TYPE cipherOperation;
> +  CK_MECHANISM_TYPE cipherMech = CKM_GENERIC_SECRET_KEY_GEN;
> +  CK_ATTRIBUTE_TYPE cipherOperation = CKA_SIGN;

Style Q: Since these are constants now, should we even bother keeping them declared as locals? A cursory scan through `manager/ssl/` didn't turn up anything quite like this.

::: security/manager/ssl/nsKeyModule.cpp
(Diff revision 1)
> -    NS_ERROR("no slot");

Just for my own knowledge: The couple of places where you've removed the NS_ERROR, this is because we generally don't want to just halt anymore, yes?

Comment 5

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

https://reviewboard.mozilla.org/r/27809/#review26237

> Style Q: Since these are constants now, should we even bother keeping them declared as locals? A cursory scan through `manager/ssl/` didn't turn up anything quite like this.

Looks like cipherMech is used twice here, so I thought it made sense to keep it as a local (particularly since CKM_GENERIC_SECRET_KEY_GEN is so long). I suppose I could replace the one use of cipherOperation with CKA_SIGN, but it didn't seem important one way or another. I suppose really they should be const. I'll maybe update those if I have to change anything else.

> Just for my own knowledge: The couple of places where you've removed the NS_ERROR, this is because we generally don't want to just halt anymore, yes?

Generally, yes. In this particular case, I think that call could fail in some strange configuration (e.g. misbehaving PKCS11 modules), so it shouldn't be fatal anyway. It'll just throw a JS exception.

Comment 6

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

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

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6a05835d04
https://hg.mozilla.org/integration/mozilla-inbound/rev/705dc2c96e94

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7e6a05835d04
https://hg.mozilla.org/mozilla-central/rev/705dc2c96e94