1239421 - don't go through XPCOM for instantiating nsI{CryptoHash,KeyObject,KeyObjectFactory} things

Status: RESOLVED DUPLICATE of bug 1773797

(Core :: Security: PSM, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - create nsIKeyObject{,Factory} instances directory

There's no need for things in security/manager/ssl/ to go through XPCOM
to create instances of nsIKeyObject{,Factory}.

Furthermore, the code in nsNTLMAuthModule.cpp was creating nsIKeyObject
objects only to blow them away later with calls to
nsIKeyObjectFactory::KeyFromString.  That's useless work, so let's just
get rid of it.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - create nsICryptoHash instances directly

Comment 3

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

Comment on attachment 8707572 [details] [diff] [review]
part 1 - create nsIKeyObject{,Factory} instances directory

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

This looks good, but one concrete change as a result of not going through XPCOM is that these objects won't ensure that NSS has been initialized. If I'm reading this code correctly, though, these sites can only be reached through something else that will first ensure that NSS has been initialized, so I think we're good there. It might be nice to include a comment to this effect, though.

Comment 4

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

Comment on attachment 8707573 [details] [diff] [review]
part 2 - create nsICryptoHash instances directly

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

::: security/manager/ssl/CertBlocklist.cpp
@@ +580,5 @@
>              ("certblocklist::IsCertRevoked found by issuer / serial"));
>      return NS_OK;
>    }
>  
> +  nsCOMPtr<nsICryptoHash> crypto = new nsCryptoHash();

Same idea that this won't guarantee NSS has been initialized, but currently on all code paths to this point it will have been. Just a comment here should be fine.

Comment 5

[Nathan Froyd [:froydnj]]

Ah, I didn't realize there were other constraints at play here.  Thanks for pointing those out!

Just having comments seems a bit fragile, though.  It seems better to do:

class nsCryptoHash
{
public:
  // This does all the NS_NSS_GENERIC_FACTORY_CONSTRUCTOR* stuff from
  // nsNSSModule.cpp.
  static already_AddRefed<nsCryptoHash> Create();
};

and then call |Create()| from the bits in nsNSSModule.cpp.  I'm not sure it can be made to work with the |ensureOperator| bits from NS_NSS_GENERIC_FACTORY_CONSTRUCTOR*, but I'd like to figure out a way to stop calling the component manager from off the main thread.  And that means there are going to be more of these sorts of patches to security/, so it seems worthwhile to try and get things right.

WDYT?

Comment 6

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

(In reply to Nathan Froyd [:froydnj] from comment #5)
> I'd like to figure out a way to
> stop calling the component manager from off the main thread.

The first NSS initialization has to happen on the main thread as well, so if any of these classes are instantiated off the main thread before NSS has been initialized, things will fail (of course, if NSS has already been initialized, it doesn't matter either way).

There's an ongoing project to rework how all of this initialization happens (more info here: https://wiki.mozilla.org/SecurityEngineering/NSS_Startup_and_Shutdown_in_Gecko ). Part of the plan is to set it up so we don't even need the special NS_NSS_GENERIC_FACTORY_* junk (basically by guaranteeing that NSS will be initialized early in startup). Maybe this can be part of that work?

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:dveditz, could you have a look please?
For more information, please visit auto_nag documentation.