1039642 - smart card monitor threads not unloading/ending properly

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

While working on bug 1038913, I discovered that even after unloading a PKCS11 module, it can generate smart card events (as in, it appears that the smart card monitor thread still polls for events). This may be due to a bug in the test module implementation, but in any case it should be investigated.

Comment 1

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

This appears to be a bug in the original implementation. This is nsPkcs11::DeleteModule:

52   NS_ConvertUTF16toUTF8 modName(aModuleName);
53   int32_t modType;
54   SECStatus srv = SECMOD_DeleteModule(modName.get(), &modType);
55   if (srv == SECSuccess) {
56     SECMODModule *module = SECMOD_FindModule(modName.get());
57     if (module) {
58 #ifndef MOZ_NO_SMART_CARDS
59       nssComponent->ShutdownSmartCardThread(module);
60 #endif
61       SECMOD_DestroyModule(module);
62     }

Note that we're attempting to find the given module (line 56) after we've already deleted it (line 54). This obviously fails, so nssComponent->ShutdownSmartCardThread never gets called, and the thread lives on until shutdown. For posterity, this is how the code originally landed: https://github.com/jrmuizel/mozilla-cvs-history/commit/83a733d67f56a3909a2200625a1c8969be323840#diff-f9895193ae48745689ce0aaf156bd08dR2570

Comment 2

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

Attachment: patch 1/3: fix

Comment 3

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

Attachment: patch 2/3: test

Comment 4

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

Attachment: patch 3/3: cleanup

Comment 5

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

Comment on attachment 8516341 [details] [diff] [review]
patch 1/3: fix

Review of attachment 8516341 [details] [diff] [review]:
-----------------------------------------------------------------

keeler: this looks fine and is simply easier to understand, so ... bonus.

Comment 6

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

Comment on attachment 8516342 [details] [diff] [review]
patch 2/3: test

Review of attachment 8516342 [details] [diff] [review]:
-----------------------------------------------------------------

If my nit results in an update, carry r+ forward with gusto!

::: security/manager/ssl/tests/unit/test_pkcs11_no_events_after_removal.js
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  // This test loads a testing PKCS #11 module that simulates a token being
>  // inserted and removed from a slot every 50ms. This causes the observer

Nit, I guess? Is it still doing this every 50 millis? It appears to only fire once.

There's a very real chance I simply don't know something magic about how do_test_pending() works, so this is either a nitpick about a stale comment, or an introspective comment about my stale understanding. :)

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8516341 [details] [diff] [review]
patch 1/3: fix

Review of attachment 8516341 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsCrypto.cpp
@@ +55,5 @@
> +  if (!module) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  nssComponent->ShutdownSmartCardThread(module);
> +  SECMOD_DestroyModule(module);

I'm reading this out of context, but it's not obvious what the correct ordering of SECMOD_DestroyModule and SECMOD_DeleteModule is. If this order is important, it may deserve a comment. In the cvs commit you pointed to, there's another instance of Delete followed by Destroy.

Comment 8

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8516343 [details] [diff] [review]
patch 3/3: cleanup

Review of attachment 8516343 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsCrypto.cpp
@@ +12,3 @@
>  #include "secmod.h"
>  
> +typedef ScopedPtr<SECMODModule, SECMOD_DestroyModule> ScopedSECMODModule;

Yay!

::: security/manager/ssl/src/nsCrypto.h
@@ +13,5 @@
>  #define NS_PKCS11_CID \
>    {0x74b7a390, 0x3b41, 0x11d4, { 0x8a, 0x80, 0x00, 0x60, 0x08, 0xc8, 0x44, 0xc3} }
>  
>  class nsPkcs11 : public nsIPKCS11
> +               , public nsNSSShutDownObject

Hmm, seems like this might have caused shutdown crashes before... Is there any easy way to make sure everyone's subclassing shutdown object who needs to?

Comment 9

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

Comment on attachment 8516343 [details] [diff] [review]
patch 3/3: cleanup

Review of attachment 8516343 [details] [diff] [review]:
-----------------------------------------------------------------

I am so unfamiliar that I don't know that this review counts for much, but I don't see anything that stands out as a problem.

Comment 10

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

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #7)
> Comment on attachment 8516341 [details] [diff] [review]
> patch 1/3: fix
> 
> Review of attachment 8516341 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/manager/ssl/src/nsCrypto.cpp
> @@ +55,5 @@
> > +  if (!module) {
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +  nssComponent->ShutdownSmartCardThread(module);
> > +  SECMOD_DestroyModule(module);
> 
> I'm reading this out of context, but it's not obvious what the correct
> ordering of SECMOD_DestroyModule and SECMOD_DeleteModule is. If this order
> is important, it may deserve a comment. In the cvs commit you pointed to,
> there's another instance of Delete followed by Destroy.

SECMOD_FindModule causes a SECMODModule to be "constructed", and SECMOD_DestroyModule is its "destructor", so those have to be paired. In any case, that essentially creates a handle on a module so it can be manipulated. Even outside of the scope of a FindModule/DestroyModule pair, if a module has been loaded, it still exists in the backend. SECMOD_DeleteModule (if I'm understanding the code correctly) removes a module from the backend entirely. So, calling SECMOD_FindModule/SECMOD_DestroyModule after a module has been deleted with SECMOD_DeleteModule makes no sense, which is what the bug was, here.

Comment 11

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

(In reply to J.C. Jones [:jcj] from comment #6)
> Comment on attachment 8516342 [details] [diff] [review]
> patch 2/3: test
> 
> Review of attachment 8516342 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If my nit results in an update, carry r+ forward with gusto!
> 
> ::: security/manager/ssl/tests/unit/test_pkcs11_no_events_after_removal.js
> @@ +3,5 @@
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  // This test loads a testing PKCS #11 module that simulates a token being
> >  // inserted and removed from a slot every 50ms. This causes the observer
> 
> Nit, I guess? Is it still doing this every 50 millis? It appears to only
> fire once.

The firing every 50ms actually happens here: http://hg.mozilla.org/mozilla-central/annotate/ffa0f66db411/security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp#l490

> There's a very real chance I simply don't know something magic about how
> do_test_pending() works, so this is either a nitpick about a stale comment,
> or an introspective comment about my stale understanding. :)

do_test_pending() tells the test harness that eventually an event will happen that causes do_test_finished() to be called, so when it returns to the event loop it waits for that to happen instead of exiting the test. More information here: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

Comment 12

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

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #8)
> ::: security/manager/ssl/src/nsCrypto.h
> @@ +13,5 @@
> >  #define NS_PKCS11_CID \
> >    {0x74b7a390, 0x3b41, 0x11d4, { 0x8a, 0x80, 0x00, 0x60, 0x08, 0xc8, 0x44, 0xc3} }
> >  
> >  class nsPkcs11 : public nsIPKCS11
> > +               , public nsNSSShutDownObject
> 
> Hmm, seems like this might have caused shutdown crashes before... Is there
> any easy way to make sure everyone's subclassing shutdown object who needs
> to?

There's only one place where these functions actually get called in vanilla Firefox, and I don't think it's possible for them to run during shutdown. So, I think only a misbehaving addon would have been able to cause crashes as a result of this, but it's still the right thing to do. Unfortunately, short of shimming all calls to NSS, I don't think there's an automated way to enforce what we need to, here.

Comment 13

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

(In reply to J.C. Jones [:jcj] from comment #9)
> Comment on attachment 8516343 [details] [diff] [review]
> patch 3/3: cleanup
> 
> Review of attachment 8516343 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am so unfamiliar that I don't know that this review counts for much, but I
> don't see anything that stands out as a problem.

Well, part of the idea is to get you up to speed with this area of code. It's also another check to make sure I'm not making any obvious mistakes :)

Comment 14

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

Thanks for the reviews!

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ffcf3db82a12

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fce62bbe2ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/c01c360b2ccd
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb6355a9cdc0

Comment 15

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

Non-unified build bustage fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87cc8b0a2596

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9fce62bbe2ff
https://hg.mozilla.org/mozilla-central/rev/c01c360b2ccd
https://hg.mozilla.org/mozilla-central/rev/bb6355a9cdc0
https://hg.mozilla.org/mozilla-central/rev/87cc8b0a2596