Bug 1625791 - Call STAN_GetCERTCertificate to load CERTCertificate trust before caching. r=jcj,keeler
authorKevin Jacobs <kjacobs@mozilla.com>
Fri, 07 Aug 2020 22:15:00 +0000
changeset 15731 c06f22733446c6fb55362b9707fa714c15caf04e
parent 15730 41ecb7fe55461da66328758a08776f2291ea4d0b
child 15732 2890f342de631bf6774ac747515a8b5736e20d3f
push id3821
push userkjacobs@mozilla.com
push dateFri, 07 Aug 2020 22:18:04 +0000
reviewersjcj, keeler
bugs1625791, 1651564
Bug 1625791 - Call STAN_GetCERTCertificate to load CERTCertificate trust 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
lib/pki/tdcache.c
--- a/lib/pki/tdcache.c
+++ b/lib/pki/tdcache.c
@@ -69,17 +69,17 @@ log_cert_ref(const char *msg, NSSCertifi
 
 /* XXX
  * Locking is not handled well at all.  A single, global lock with sub-locks
  * in the collection types.  Cleanup needed.
  */
 
 /* 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;
     nssHash *nickname;
     nssHash *email;
 };
 
 struct cache_entry_str {
@@ -708,16 +708,24 @@ add_cert_to_cache(
     NSSArena *arena = NULL;
     nssList *subjectList = NULL;
     PRStatus nssrv;
     PRUint32 added = 0;
     cache_entry *ce;
     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);
     if (ce) {
         ce->hits++;
         ce->lastHit = PR_Now();
         rvCert = nssCertificate_AddRef(ce->entry.cert);
 #ifdef DEBUG_CACHE