911336 - nsNSSCertificateDB does not correctly check for nssShutdown

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Camilo Viecco (:cviecco)]

While NSSCertificateDB uses the shutdown preverntion locker, it does not check that nss has not initiated shutdown.

The class must be derived from nsNSSShutDownObject and call isAlreadyShutDown() before using nss resources.

This can be easily done for all but static member functions.

Comment 1

[Camilo Viecco (:cviecco)]

Attachment: fix-shutdown-nsscertificatedb

Comment 2

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

Comment on attachment 798023 [details] [diff] [review]
fix-shutdown-nsscertificatedb

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

So, is it the case that any object that calls an nss function needs to do this shutdown prevention/checking?
Anyway, there are some unresolved questions I think Brian (or someone on the nss team, maybe) can answer.

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +73,5 @@
> +void nsNSSCertificateDB::virtualDestroyNSSReference()
> +{
> +  destructorSafeDestroyNSSReference();
> +}
> + 

unnecessary space

@@ +741,5 @@
>  nsNSSCertificateDB::ImportValidCACerts(int numCACerts, SECItem *CACerts, nsIInterfaceRequestor *ctx)
>  {
> +
> +  nsNSSShutDownPreventionLock locker;
> +/* XXX Static Function, cannot use members.

I would ask Brian what to do about these.

@@ +845,5 @@
>  void nsNSSCertificateDB::DisplayCertificateAlert(nsIInterfaceRequestor *ctx, 
>                                                   const char *stringID, 
>                                                   nsIX509Cert *certToShow)
>  {
> +  //XXX no lock?

I would just remove this comment - it's pretty clear this function needs NSS to not shut down.

@@ +1254,3 @@
>    uint32_t numcerts = 0, i=0;
>    PRUnichar **tmpArray = nullptr;
> +  int nc = 0;

I'm not sure why you moved these declarations around, but I'm not sure it matters.

@@ +1610,5 @@
>  {
>    NS_ENSURE_ARG_POINTER(aBase64);
>    nsCOMPtr <nsIX509Cert> newCert;
>  
> +  // NO locks?

Again, probably don't need a comment here (and same with the rest).

::: security/manager/ssl/src/nsNSSCertificateDB.h
@@ +15,5 @@
>  class nsCString;
>  class nsIArray;
>  class nsRecentBadCerts;
>  
> +class nsNSSCertificateDB : public nsIX509CertDB, 

Trailing space. Also, I think it's more common to do this:

class nsNSSCertificateDB : public nsIX509CertDB
                         , public nsIX509CertDB2
                         , public nsNSSShutdownObject

(Lining up everything properly, of course. This makes it easier to add/delete lines.)

@@ +59,5 @@
>    mozilla::Mutex mBadCertsLock;
>    mozilla::RefPtr<nsRecentBadCerts> mPublicRecentBadCerts;
>    mozilla::RefPtr<nsRecentBadCerts> mPrivateRecentBadCerts;
> +
> +  virtual void virtualDestroyNSSReference();

You might do what this does:
https://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/BackgroundFileSaver.h#361

Comment 3

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

Comment on attachment 798023 [details] [diff] [review]
fix-shutdown-nsscertificatedb

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

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +1493,5 @@
>    nickname.Truncate();
>  
>    nsNSSShutDownPreventionLock locker;
> +/* XXX this is a static function and does not have access to this
> +   Fortunatelly is is only called from nsNScripto. 

Forgot to mention that there are some typos on this line, as well as a trailing space.

Comment 4

[Camilo Viecco (:cviecco)]

(In reply to David Keeler (:keeler) from comment #2)
> Comment on attachment 798023 [details] [diff] [review]
> fix-shutdown-nsscertificatedb
> 
> Review of attachment 798023 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So, is it the case that any object that calls an nss function needs to do
> this shutdown prevention/checking?
Yes.. they should.

> Anyway, there are some unresolved questions I think Brian (or someone on the
> nss team, maybe) can answer.
> 
> ::: security/manager/ssl/src/nsNSSCertificateDB.cpp
> @@ +73,5 @@
> > +void nsNSSCertificateDB::virtualDestroyNSSReference()
> > +{
> > +  destructorSafeDestroyNSSReference();
> > +}
> > + 
> 
> unnecessary space
> 
> @@ +741,5 @@
> >  nsNSSCertificateDB::ImportValidCACerts(int numCACerts, SECItem *CACerts, nsIInterfaceRequestor *ctx)
> >  {
> > +
> > +  nsNSSShutDownPreventionLock locker;
> > +/* XXX Static Function, cannot use members.
> 
> I would ask Brian what to do about these.
> 
> @@ +845,5 @@
> >  void nsNSSCertificateDB::DisplayCertificateAlert(nsIInterfaceRequestor *ctx, 
> >                                                   const char *stringID, 
> >                                                   nsIX509Cert *certToShow)
> >  {
> > +  //XXX no lock?
> 
> I would just remove this comment - it's pretty clear this function needs NSS
> to not shut down.
> 
> @@ +1254,3 @@
> >    uint32_t numcerts = 0, i=0;
> >    PRUnichar **tmpArray = nullptr;
> > +  int nc = 0;
> 
> I'm not sure why you moved these declarations around, but I'm not sure it
> matters.
When I added the early termination, this funcion terminates with a goto. And
variable declarations cannot cross gotos.

> 
> @@ +1610,5 @@
> >  {
> >    NS_ENSURE_ARG_POINTER(aBase64);
> >    nsCOMPtr <nsIX509Cert> newCert;
> >  
> > +  // NO locks?
my bad this function (as well as others) had no locking/shutdown prevention
I marked them with this I I tough I had clean up everything,
> 
> Again, probably don't need a comment here (and same with the rest).
> 
> ::: security/manager/ssl/src/nsNSSCertificateDB.h
> @@ +15,5 @@
> >  class nsCString;
> >  class nsIArray;
> >  class nsRecentBadCerts;
> >  
> > +class nsNSSCertificateDB : public nsIX509CertDB, 
> 
> Trailing space. Also, I think it's more common to do this:
> 
> class nsNSSCertificateDB : public nsIX509CertDB
>                          , public nsIX509CertDB2
>                          , public nsNSSShutdownObject
> 
> (Lining up everything properly, of course. This makes it easier to
> add/delete lines.)
> 
> @@ +59,5 @@
> >    mozilla::Mutex mBadCertsLock;
> >    mozilla::RefPtr<nsRecentBadCerts> mPublicRecentBadCerts;
> >    mozilla::RefPtr<nsRecentBadCerts> mPrivateRecentBadCerts;
> > +
> > +  virtual void virtualDestroyNSSReference();
> 
> You might do what this does:
> https://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/
> BackgroundFileSaver.h#361

Thanks, yes this is much clearer

Comment 5

[Camilo Viecco (:cviecco)]

Attachment: fix-shutdown-nsscertificatedb (v 2)

Comment 6

[Camilo Viecco (:cviecco)]

Attachment: fix-shutdown-nsscertificatedb (v 2.1)

Comment 7

[Camilo Viecco (:cviecco)]

Attachment: cleanup-for-nsscertificatedb (v 2.1)

Comment 8

[Camilo Viecco (:cviecco)]

Comment on attachment 799020 [details] [diff] [review]
cleanup-for-nsscertificatedb (v 2.1)

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

After thinking it more, I think I actually need to do this cleanup. So this is on top of the other one.

Comment 9

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

Comment on attachment 799019 [details] [diff] [review]
fix-shutdown-nsscertificatedb (v 2.1)

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

Still concerned about the static functions issue, but otherwise I think this looks good.

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +728,5 @@
>  nsNSSCertificateDB::ImportValidCACerts(int numCACerts, SECItem *CACerts, nsIInterfaceRequestor *ctx)
>  {
> +
> +  nsNSSShutDownPreventionLock locker;
> +/* XXX Static Function, cannot use members.

Is there a solution for this, or are we leaving this to a follow-up bug?

@@ +1479,5 @@
>    nickname.Truncate();
>  
>    nsNSSShutDownPreventionLock locker;
> +/* XXX this is a static function and does not have access to this
> + * Fortunately is is only called from nsNScripto.

Do you mean nsCrypto?

Comment 10

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

Comment on attachment 799020 [details] [diff] [review]
cleanup-for-nsscertificatedb (v 2.1)

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

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +76,5 @@
> +}
> +
> +void nsNSSCertificateDB::destructorSafeDestroyNSSReference()
> +{
> +  if (isAlreadyShutDown())

Braces around the conditional body.

@@ +78,5 @@
> +void nsNSSCertificateDB::destructorSafeDestroyNSSReference()
> +{
> +  if (isAlreadyShutDown())
> +    return;
> +  // Bsmith: my guess is we have to clean up this.

Sounds good. We probably don't need this comment, though.

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 799019 [details] [diff] [review]
fix-shutdown-nsscertificatedb (v 2.1)

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

I suggest we follow this pattern:

1. Every public/exported function that deals with NSS APIs should be responsible for acquiring the shutdown prevention lock and checking that NSS hasn't been shut down.
2. Every non-public/non-exported function that deals with NSS APIs should take a const nsNSSShutDownPreventionLock& /*proofOfLock*/ parameter. These functions DO NOT need to check that NSS hasn't been shut down; we can assume the caller did the check.

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +728,5 @@
>  nsNSSCertificateDB::ImportValidCACerts(int numCACerts, SECItem *CACerts, nsIInterfaceRequestor *ctx)
>  {
> +
> +  nsNSSShutDownPreventionLock locker;
> +/* XXX Static Function, cannot use members.

Add a const nsNSSShutdownPreventionLock& /*proofOfLock*/ parameter. Then change the callers to pass in their lock.

@@ +1242,5 @@
> +  int nc = 0;
> +
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown()) {
> +    goto finish;

Please don't use goto when it is so easy to avoid it.

Also, this is not a public function, so you don't need to do this check here. Instead, you can do the same proofOfLock thing as I mentioned above. This is more efficient as it avoids re-entering the monitor and avoid doing duplicate checks.

@@ +1483,5 @@
> + * Fortunately is is only called from nsNScripto.
> +  if (isAlreadyShutDown()) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +*/

Ditto.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 799020 [details] [diff] [review]
cleanup-for-nsscertificatedb (v 2.1)

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

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +86,5 @@
> +      mPublicRecentBadCerts->ResetStoredCerts();
> +    }
> +    if(mPrivateRecentBadCerts) {
> +      mPrivateRecentBadCerts->ResetStoredCerts();
> +    }

mPublicRecentBadCerts and mPrivaterecentBadCerts are instances of nsRecentBadCerts. If nsRecentBadCerts needed to be concerned with NSS shutdown then it should implement nsNSShutDownObject itself. However, if you look at the implementation of nsRecentBadCerts you can see that it doesn't use any store any NSS objects (no CERTCertificate* for example) so it doesn't need to be concerned about NSS shutdown.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

See also bug 926251. SECITEM_* is in NSS but we don't have to ensure it is called between NSS_Init and NSS_Shutdown.

Comment 14

[Camilo Viecco (:cviecco)]

Attachment: fix-shutdown-nsscertificatedb (v 3)

Comment 15

[Camilo Viecco (:cviecco)]

Attachment: fix-shutdown-nsscertificatedb (v 3.1)

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 818559 [details] [diff] [review]
fix-shutdown-nsscertificatedb (v 3.1)

Can't get to this before I leave for a few days. David, please read my previous review comments and do the review.

Comment 17

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

Comment on attachment 818559 [details] [diff] [review]
fix-shutdown-nsscertificatedb (v 3.1)

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

This is a good solution. Just a couple of issues, though, so r- for now.
As an overall comment, there are some lines that are now more than 80 characters due to parameter alignment, etc. You might see what you can do about that.

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +236,5 @@
>  
>  CERTDERCerts*
>  nsNSSCertificateDB::getCertsFromPackage(PLArenaPool *arena, uint8_t *data, 
> +                                        uint32_t length,
> +                                        const nsNSSShutDownPreventionLock &proofOfLock)

If you use /* */ to comment out "proofOfLock", we can avoid "unused parameter" warnings.

@@ +764,4 @@
>  }
>  
>  nsresult
> +nsNSSCertificateDB::ImportValidCACertsInList(CERTCertList *certList, nsIInterfaceRequestor *ctx, 

trailing space

@@ +808,5 @@
>  
>  void nsNSSCertificateDB::DisplayCertificateAlert(nsIInterfaceRequestor *ctx, 
>                                                   const char *stringID, 
> +                                                 nsIX509Cert *certToShow,
> +                                                 const nsNSSShutDownPreventionLock &proofOfLock)

again, if this function doesn't pass on the proofOfLock, its name can be commented-out

@@ +1149,5 @@
>                                       nsIFile *aFile)
>  {
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown()) {
> +    return NS_ERROR_NOT_AVAILABLE;

It looks like this function doesn't directly use NSS functions. The nsPKCS12Blob/nsIPK11Token implementations should take care of NSS shutdown themselves, right? If so, I don't think we need the lock/check here (do correct me if I'm wrong - this is definitely something we should clear up). (Of course, if those implementations don't do the right thing themselves, we should fix that, too.)

@@ +1207,5 @@
>  nsNSSCertificateDB::getCertNames(CERTCertList *certList,
>                                   uint32_t      type, 
>                                   uint32_t     *_count,
> +                                 PRUnichar  ***_certNames,
> +                                 const nsNSSShutDownPreventionLock &proofOfLock)

same here for commenting out "proofOfLock"

@@ +1441,5 @@
>  void
>  nsNSSCertificateDB::get_default_nickname(CERTCertificate *cert, 
>                                           nsIInterfaceRequestor* ctx,
> +                                         nsCString &nickname,
> +                                         const nsNSSShutDownPreventionLock &proofOfLock)

same here

@@ +1647,5 @@
>  nsNSSCertificateDB::GetRecentBadCerts(bool isPrivate, nsIRecentBadCerts** result)
>  {
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown()) {
> +    return NS_ERROR_NOT_AVAILABLE;

Again, it doesn't look like nsNSSCertificateDB uses any NSS functions here, so we don't need the lock/check, right?

Comment 18

[Camilo Viecco (:cviecco)]

Comment on attachment 818559 [details] [diff] [review]
fix-shutdown-nsscertificatedb (v 3.1)

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

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +1149,5 @@
>                                       nsIFile *aFile)
>  {
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown()) {
> +    return NS_ERROR_NOT_AVAILABLE;

They are in the same leage as nsNSSCertificateDB. They have locks, but not ensure that shutdown har not started. I would prefer leave this on untill the other ones are fixed, but is up to you.

@@ +1647,5 @@
>  nsNSSCertificateDB::GetRecentBadCerts(bool isPrivate, nsIRecentBadCerts** result)
>  {
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown()) {
> +    return NS_ERROR_NOT_AVAILABLE;

yes we can do it this way.

Comment 19

[Camilo Viecco (:cviecco)]

Attachment: fix-shutdown-nsscertificatedb (v 4)

Comment 20

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

Comment on attachment 818784 [details] [diff] [review]
fix-shutdown-nsscertificatedb (v 4)

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

Huh - it looks like nsRecentBadCerts::GetRecentBadCert does actually use NSS functions, so I take back what I said about not doing the lock/check in nsNSSCertificateDB::GetRecentBadCerts. r+ with that put back in.

Comment 21

[Camilo Viecco (:cviecco)]

Attachment: fix-shutdown-nsscertificatedb (v 4.1)

keeping r+ from v4

Comment 22

[Camilo Viecco (:cviecco)]

 https://tbpl.mozilla.org/?tree=Try&rev=2c2a8dd7a177

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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