Closed Bug 911336 Opened 7 years ago Closed 6 years ago

nsNSSCertificateDB does not correctly check for nssShutdown

Categories

(Core :: Security: PSM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: cviecco, Assigned: cviecco)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 7 obsolete files)

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.
QA Contact: cviecco
Assignee: nobody → cviecco
QA Contact: cviecco
Attached patch fix-shutdown-nsscertificatedb (obsolete) — Splinter Review
Attachment #798023 - Flags: review?(dkeeler)
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
Attachment #798023 - Flags: review?(dkeeler) → review-
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.
(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
Attachment #798023 - Attachment is obsolete: true
Attachment #798917 - Attachment is obsolete: true
Attachment #799019 - Flags: review?(dkeeler)
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.
Attachment #799020 - Flags: review?(dkeeler)
Blocks: 912155
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?
Attachment #799019 - Flags: review?(dkeeler) → review+
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.
Attachment #799020 - Flags: review?(dkeeler) → review+
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.
Attachment #799019 - Flags: review-
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.
Attachment #799020 - Flags: review-
See also bug 926251. SECITEM_* is in NSS but we don't have to ensure it is called between NSS_Init and NSS_Shutdown.
Attachment #799019 - Attachment is obsolete: true
Attachment #799020 - Attachment is obsolete: true
Attachment #818551 - Attachment is obsolete: true
Attachment #818559 - Flags: review?(brian)
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.
Attachment #818559 - Flags: review?(brian) → review?(dkeeler)
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?
Attachment #818559 - Flags: review?(dkeeler) → review-
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.
Attachment #818559 - Attachment is obsolete: true
Attachment #818784 - Flags: review?(dkeeler)
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.
Attachment #818784 - Flags: review?(dkeeler) → review+
keeping r+ from v4
Attachment #818784 - Attachment is obsolete: true
Attachment #819115 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f78f52c8c9ff
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.