1381154 - remove smartcard insertion/removal monitoring threads

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Description

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

Currently, if the user has any PKCS#11 modules loaded and smartcard devices with removal tokens attached, Firefox will spin up a thread for each such module to monitor insertion and removal events. However, the only functionality provided by this non-trivial infrastructure is to refresh the certificate manager and device manager when an event is detected. Since this provides so little at considerable cost (see the hangs at https://crash-stats.mozilla.com/search/?signature=~SmartCardMonitoringThread ), we should just remove this.

(NB: We should figure out what happens in the certificate manager when the user attempts to interact with a certificate that only existed on a token that has been removed.)

(Also, it looks like products based on comm-central have similar consumers of this mechanism that e.g. reload a message, so they'll have to consider what they want to do there.)

Comment 1

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

To loop back with myself on this, it turns out NSS doesn't notice if a token has a new slot inserted, which consequently means that any certificates or keys provided by that slot aren't available unless PK11_IsPresent is called. The monitoring threads handle this in a reasonable way (even if the implementation is suspect) in that idly waiting for an event and then calling PK11_IsPresent is more efficient than calling it every time we (e.g.) search for certificates. This bug will probably end up being WONTFIX or perhaps morphing into an implementation cleanup bug.

Comment 2

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 3

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

Ehhhh let's wontfix this for now.

Comment 4

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

This may address bug 1248818.

Comment 5

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

Attachment: bug 1381154 - remove smartcard monitoring threads

Modified from bug 1248818 comment 11:
Before this patch, if a user had a smart card (PKCS#11 device) with removable
slots, Firefox would launch a thread for each module and loop, calling
SECMOD_WaitForAnyTokenEvent to be alerted to any insertions/removals. At
shutdown, we would call SECMOD_CancelWait, which would cancel any waiting
threads. However, since that involved calling 3rd party code, we really had no
idea if these modules were behaving correctly (and, indeed, they often weren't,
judging by the shutdown crashes we were getting).
The real solution is to stop relying on PKCS#11, but since that's unlikely in
the near future, the next best thing would be to load these modules in a child
process. That way, misbehaving modules don't cause Firefox to hang/crash/etc.
That's a lot of engineering work, though, so what this patch does is avoids the
issue by never calling SECMOD_WaitForAnyTokenEvent (and thus we never have to
call SECMOD_CancelWait, etc.). Instead, every time Firefox performs an operation
that may be affected by a newly added or removed smart card, it first has NSS
refresh its view of any removable slots. This is similar to how we ensure the
loadable roots module has been loaded (see bug 1372656).

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

Comment 6

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

Comment on attachment 8919074 [details]
bug 1381154 - remove smartcard monitoring threads

https://reviewboard.mozilla.org/r/189988/#review195156

::: security/manager/pki/resources/content/device_manager.js:16
(Diff revision 1)
>  const nsPK11TokenDB = "@mozilla.org/security/pk11tokendb;1";
>  const nsIPK11TokenDB = Components.interfaces.nsIPK11TokenDB;
>  const nsIDialogParamBlock = Components.interfaces.nsIDialogParamBlock;
>  const nsDialogParamBlock = "@mozilla.org/embedcomp/dialogparam;1";
>  
>  var { Services } = Components.utils.import("resource://gre/modules/Services.jsm", {});

Looks like `Services` is now unused.

::: security/manager/ssl/nsNSSComponent.cpp:1164
(Diff revision 1)
> +      list = list->next;
> +    }
> +  }
> +  for (auto& module : modulesWithRemovableSlots) {
> +    // Best-effort.
> +    Unused << SECMOD_UpdateSlotList(module.get());
> +    for (int i = 0; i < module->slotCount; i++) {

Do we need to have some sort of synchronization to worry about modules that are removed between the population of `modulesWithRemovableSlots` and the loop through it?

Comment 7

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

Comment on attachment 8919074 [details]
bug 1381154 - remove smartcard monitoring threads

https://reviewboard.mozilla.org/r/189988/#review195156

> Looks like `Services` is now unused.

Righto.

> Do we need to have some sort of synchronization to worry about modules that are removed between the population of `modulesWithRemovableSlots` and the loop through it?

In theory a new module could be added (and a new token inserted) in between when the list is created and when we refresh our view of it. If whatever operation in progress depends on something on that token, the operation will fail. However, this would be the same as if the token had been inserted too late with respect to the operation being done (e.g. connecting to a site with 3rd party roots, connecting to a site requiring a client certificate, etc.). Since these operations can all be retried, I don't think it's a concern.

Comment 8

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

Comment on attachment 8919074 [details]
bug 1381154 - remove smartcard monitoring threads

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189988/diff/1-2/

Comment 9

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

Comment on attachment 8919074 [details]
bug 1381154 - remove smartcard monitoring threads

https://reviewboard.mozilla.org/r/189988/#review196372

OK, thanks!

Comment 10

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8919074 [details]
bug 1381154 - remove smartcard monitoring threads

https://reviewboard.mozilla.org/r/189988/#review196746

Comment 11

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

Thanks for the reviews!
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fd2e1ee05c0

Comment 12

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 0b3bedb96285 -d cc74db25e470: rebasing 428726:0b3bedb96285 "bug 1381154 - remove smartcard monitoring threads r=jcj,mgoodwin" (tip)
merging security/certverifier/CertVerifier.cpp
merging security/manager/ssl/PKCS11ModuleDB.cpp
merging security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp
merging security/manager/ssl/tests/unit/xpcshell-smartcards.ini
warning: conflicts while merging security/manager/ssl/tests/unit/xpcshell-smartcards.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 13

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

Comment on attachment 8919074 [details]
bug 1381154 - remove smartcard monitoring threads

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189988/diff/2-3/

Comment 14

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

Rebased.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b15691743aba
remove smartcard monitoring threads r=jcj,mgoodwin

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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