108600 - Avoid the risk of unexpected NSS shutdowns

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[Kai Engert [:KaiE:]]

David Baron found this problem. David, please correct if I repeat incorrectly
what Brian told me.

The bug David found can lead to NSS being shut down too early, or too often.

The reason is the PSM code that defines the nsNSSComponent module. This same
component is listed for multiple contract IDs.

Currently, nsNSSComponent is designed to be a singleton. If it is created
multiple times, we run into big problems.

While the code that explicitly asks for the nsNSSComponent uses GetService, thus
accessing the singleton if it has been already created, some of the other
contracts (e.g. the cert handlers) do not use this concept. Instead they use
CreateInstace, which leads to the problems.

Brian Ryner made the following suggestion: "My suggestion would be to split out
nsNSSComponent into separate classes. Right now it's doing way too many things,
and then some of those classes would not need to be singletons."

For details, look at file nsNSSModule.cpp. Several classes use
NS_NSSCOMPONENT_CID / nsNSSComponentConstructor.

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I described the problem in a little more detail in attachment 55670 [details] [diff] [review] on bug
75947, in the comment in nsNSSModule.cpp near the end of the patch.

Comment 2

[Kai Engert [:KaiE:]]

From the mentioned patch, a comment for the contract IDs in nsNSSModule.cpp.

+  // XXXldb Isn't the |nsNSSComponent| object supposed to be a singleton?
+  // These contract IDs are used with do_CreateInstance in nsURILoader.cpp, so
+  // their use will create additional instances of the nsNSSComponent object
+  // unless we modify nsNSSComponentConstructor to cache its singleton.  (Will
+  // it then work correctly if it being used simultaneously by multiple users
+  // for this purpose?)

Comment 3

[Kai Engert [:KaiE:]]

In my opinion it is a bug that NS_NSSCOMPONENT_CID is registered to implement
the different contracts starting with NS_CONTENT_HANDLER_CONTRACTID_PREFIX, used
by nsURILoader.

nsURIILoader tries to create an instance for nsIContentHandler. However, neither
nsNSSComponent (which is currently registered for doing that), nor any other
class in security does implement this interface.

I suggest that we remove these four registrations from the code as they have no
effect. I believe that nsURILoader will try to instantiate that class, use
QueryInterface, but fail because of a lack of support for the requested interface.

The real mime type handling for downloading certs is handled by PSMContentListener.

I will attach a patch to remove that code.

Comment 4

[Kai Engert [:KaiE:]]

Attachment: Suggested fix

Together with the patch in bug 75947 this will eliminate all additional
registrations for class nsNSSComponent, fixing this bug.

Comment 5

[Kai Engert [:KaiE:]]

Javi, while we talked about this patch, you suggested to try out whether
obtaining certificates from Verisign still works. I tested it, it works.

Can you please review?

Comment 6

[Javier Delgadillo]

Should 10977 block this?

Comment 7

[Kai Engert [:KaiE:]]

Javi, I assume you meant bug 109777 ? I don't think this blocking is necessary.

Bug 109777 blocks bug 75947. I think this is sufficient, as the code removed by
this patch does currently not have any positive effect (as the registered class
does not implement nsIContentHandler), only the negative effect of causing
additional instances of nsNSSComponent being created.

Comment 8

[Javier Delgadillo]

Comment on attachment 57164 [details] [diff] [review]
Suggested fix

r=javi (since there is a new bug to ensure cert downloading works.)

Comment 9

[Christopher Blizzard (:blizzard)]

Comment on attachment 57164 [details] [diff] [review]
Suggested fix

sr=blizzard

Comment 10

[Kai Engert [:KaiE:]]

checked in => fixed

Comment 11

[John Unruh]

Verified per kaie's comment.