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
--- 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