Closed Bug 1230377 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox45 --- affected
firefox46 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
Assignee: nobody → dkeeler
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
Attachment #8702742 - Flags: review?(jjones) → review+
Attachment #8702743 - Flags: review?(jjones) → review+
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?
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.
https://hg.mozilla.org/mozilla-central/rev/7e6a05835d04
https://hg.mozilla.org/mozilla-central/rev/705dc2c96e94
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1239609
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: