Bug 1625791 - Call STAN_GetCERTCertificate to load CERTCertificate tr…

…ust before caching. r=jcj,keeler

When caching certificates, `td->cache->lock` must not be held when taking `slot->isPresentLock`. `add_cert_to_cache` holds then former when calling the sort function in `add_subject_entry`, which will [[ https://searchfox.org/mozilla-central/rev/a3b25e347e2c22207c4b369b99246e4aebf861a7/security/nss/lib/pki/certificate.c#266 | call ]] `STAN_GetCERTCertificate` -> `fill_CERTCertificateFields` when `cc->nssCertificate` [[ https://searchfox.org/mozilla-central/rev/a3b25e347e2c22207c4b369b99246e4aebf861a7/security/nss/lib/pki/pki3hack.c#923 | is NULL ]].

There are two problems with this:

  # `fill_CERTCertificateFields` may end up locking `slot->isPresentLock` (bad ordering, bug 1651564)
  # The above may happen followed by another attempt to lock `td->cache->lock`(deadlock, this bug).

By calling `STAN_GetCERTCertificate` prior to the first lock of `td->cache->lock`, we can prevent the problematic call to `fill_CERTCertificateFields` later on, because `cc->nssCertificate` will already be filled.

Differential Revision: https://phabricator.services.mozilla.com/D86423

--HG--
extra : moz-landing-system : lando

lib/pki/tdcache.c

@@ -74,7 +74,7 @@ log_cert_ref(const char *msg, NSSCertificate *c)
 
 /* should it live in its own arena? */
 struct nssTDCertificateCacheStr {
-    PZLock *lock;
+    PZLock *lock; /* Must not be held when calling nssSlot_IsTokenPresent. See bug 1625791. */
     NSSArena *arena;
     nssHash *issuerAndSN;
     nssHash *subject;
@@ -713,6 +713,14 @@ add_cert_to_cache(
     NSSCertificate *rvCert = NULL;
     NSSUTF8 *certNickname = nssCertificate_GetNickname(cert, NULL);
 
+    /* Set cc->trust and cc->nssCertificate before taking td->cache->lock.
+     * Otherwise, the sorter in add_subject_entry may eventually call
+     * nssSlot_IsTokenPresent, which must not occur while the cache lock
+     * is held. See bugs 1625791 and 1651564 for details. */
+    if (cert->type == NSSCertificateType_PKIX) {
+        (void)STAN_GetCERTCertificate(cert);
+    }
+
     PZ_Lock(td->cache->lock);
     /* If it exists in the issuer/serial hash, it's already in all */
     ce = (cache_entry *)nssHash_Lookup(td->cache->issuerAndSN, cert);