diff options
author | Kevin Jacobs <kjacobs@mozilla.com> | 2020-11-09 17:53:21 +0000 |
---|---|---|
committer | Kevin Jacobs <kjacobs@mozilla.com> | 2020-11-09 17:53:21 +0000 |
commit | 0c9c1b286a22215932c1247ae7c6bff1d4481e86 (patch) | |
tree | 547078aa6b2d8b98141b03f15dedbe170752af54 | |
parent | a5f31cf731801c079983583abbeb49de1d1a2dc0 (diff) | |
download | nss-hg-0c9c1b286a22215932c1247ae7c6bff1d4481e86.tar.gz |
Bug 1607449 - Lock cert->nssCertificate to prevent data race. r=keeler
Differential Revision: https://phabricator.services.mozilla.com/D64233
-rw-r--r-- | lib/certdb/certdb.c | 30 | ||||
-rw-r--r-- | lib/certdb/stanpcertdb.c | 14 | ||||
-rw-r--r-- | lib/pk11wrap/pk11cert.c | 7 | ||||
-rw-r--r-- | lib/pki/pki3hack.c | 11 |
4 files changed, 41 insertions, 21 deletions
diff --git a/lib/certdb/certdb.c b/lib/certdb/certdb.c index 0796fe5d7..024bd40ea 100644 --- a/lib/certdb/certdb.c +++ b/lib/certdb/certdb.c @@ -2908,16 +2908,16 @@ CERT_LockCertTrust(const CERTCertificate *cert) PZ_Lock(certTrustLock); } -static PZLock *certTempPermLock = NULL; +static PZLock *certTempPermCertLock = NULL; /* - * Acquire the cert temp/perm lock + * Acquire the cert temp/perm/nssCert lock */ void CERT_LockCertTempPerm(const CERTCertificate *cert) { - PORT_Assert(certTempPermLock != NULL); - PZ_Lock(certTempPermLock); + PORT_Assert(certTempPermCertLock != NULL); + PZ_Lock(certTempPermCertLock); } SECStatus @@ -2941,10 +2941,10 @@ cert_InitLocks(void) } } - if (certTempPermLock == NULL) { - certTempPermLock = PZ_NewLock(nssILockCertDB); - PORT_Assert(certTempPermLock != NULL); - if (!certTempPermLock) { + if (certTempPermCertLock == NULL) { + certTempPermCertLock = PZ_NewLock(nssILockCertDB); + PORT_Assert(certTempPermCertLock != NULL); + if (!certTempPermCertLock) { PZ_DestroyLock(certTrustLock); PZ_DestroyLock(certRefCountLock); certRefCountLock = NULL; @@ -2977,10 +2977,10 @@ cert_DestroyLocks(void) rv = SECFailure; } - PORT_Assert(certTempPermLock != NULL); - if (certTempPermLock) { - PZ_DestroyLock(certTempPermLock); - certTempPermLock = NULL; + PORT_Assert(certTempPermCertLock != NULL); + if (certTempPermCertLock) { + PZ_DestroyLock(certTempPermCertLock); + certTempPermCertLock = NULL; } else { rv = SECFailure; } @@ -2999,13 +2999,13 @@ CERT_UnlockCertTrust(const CERTCertificate *cert) } /* - * Free the temp/perm lock + * Free the temp/perm/nssCert lock */ void CERT_UnlockCertTempPerm(const CERTCertificate *cert) { - PORT_Assert(certTempPermLock != NULL); - PRStatus prstat = PZ_Unlock(certTempPermLock); + PORT_Assert(certTempPermCertLock != NULL); + PRStatus prstat = PZ_Unlock(certTempPermCertLock); PORT_AssertArg(prstat == PR_SUCCESS); } diff --git a/lib/certdb/stanpcertdb.c b/lib/certdb/stanpcertdb.c index e2a668bb1..1aeddebe3 100644 --- a/lib/certdb/stanpcertdb.c +++ b/lib/certdb/stanpcertdb.c @@ -311,7 +311,9 @@ __CERT_AddTempCertToPerm(CERTCertificate *cert, char *nickname, nssPKIObject_AddInstance(&c->object, permInstance); nssTrustDomain_AddCertsToCache(STAN_GetDefaultTrustDomain(), &c, 1); /* reset the CERTCertificate fields */ + CERT_LockCertTempPerm(cert); cert->nssCertificate = NULL; + CERT_UnlockCertTempPerm(cert); cert = STAN_GetCERTCertificateOrRelease(c); /* should return same pointer */ if (!cert) { CERT_MapStanError(); @@ -808,9 +810,17 @@ CERT_DestroyCertificate(CERTCertificate *cert) /* don't use STAN_GetNSSCertificate because we don't want to * go to the trouble of translating the CERTCertificate into * an NSSCertificate just to destroy it. If it hasn't been done - * yet, don't do it at all. - */ + * yet, don't do it at all + * + * cert->nssCertificate contains its own locks and refcount, but as it + * may be NULL, the pointer itself must be guarded by some other lock. + * Rather than creating a new global lock for only this purpose, share + * an existing global lock that happens to be taken near the write in + * fill_CERTCertificateFields(). The longer-term goal is to refactor + * all these global locks to be certificate-scoped. */ + CERT_LockCertTempPerm(cert); NSSCertificate *tmp = cert->nssCertificate; + CERT_UnlockCertTempPerm(cert); if (tmp) { /* delete the NSSCertificate */ NSSCertificate_Destroy(tmp); diff --git a/lib/pk11wrap/pk11cert.c b/lib/pk11wrap/pk11cert.c index 659f3a8f6..9c745d7b8 100644 --- a/lib/pk11wrap/pk11cert.c +++ b/lib/pk11wrap/pk11cert.c @@ -1148,8 +1148,11 @@ PK11_ImportCert(PK11SlotInfo *slot, CERTCertificate *cert, } /* need to get the cert as a stan cert */ - if (cert->nssCertificate) { - c = cert->nssCertificate; + CERT_LockCertTempPerm(cert); + NSSCertificate *nssCert = cert->nssCertificate; + CERT_UnlockCertTempPerm(cert); + if (nssCert) { + c = nssCert; } else { c = STAN_GetNSSCertificate(cert); if (c == NULL) { diff --git a/lib/pki/pki3hack.c b/lib/pki/pki3hack.c index eac4a5705..7fe9113e4 100644 --- a/lib/pki/pki3hack.c +++ b/lib/pki/pki3hack.c @@ -866,9 +866,9 @@ fill_CERTCertificateFields(NSSCertificate *c, CERTCertificate *cc, PRBool forced CERT_LockCertTempPerm(cc); cc->istemp = PR_FALSE; /* CERT_NewTemp will override this */ cc->isperm = PR_TRUE; /* by default */ - CERT_UnlockCertTempPerm(cc); /* pointer back */ cc->nssCertificate = c; + CERT_UnlockCertTempPerm(cc); if (trust) { /* force the cert type to be recomputed to include trust info */ PRUint32 nsCertType = cert_ComputeCertType(cc); @@ -919,7 +919,10 @@ stan_GetCERTCertificate(NSSCertificate *c, PRBool forceUpdate) nss_SetError(NSS_ERROR_INTERNAL_ERROR); goto loser; } - if (!cc->nssCertificate || forceUpdate) { + CERT_LockCertTempPerm(cc); + NSSCertificate *nssCert = cc->nssCertificate; + CERT_UnlockCertTempPerm(cc); + if (!nssCert || forceUpdate) { fill_CERTCertificateFields(c, cc, forceUpdate); } else if (CERT_GetCertTrust(cc, &certTrust) != SECSuccess) { CERTCertTrust *trust; @@ -1018,7 +1021,9 @@ STAN_GetNSSCertificate(CERTCertificate *cc) nssCryptokiInstance *instance; nssPKIObject *pkiob; NSSArena *arena; + CERT_LockCertTempPerm(cc); c = cc->nssCertificate; + CERT_UnlockCertTempPerm(cc); if (c) { return c; } @@ -1083,7 +1088,9 @@ STAN_GetNSSCertificate(CERTCertificate *cc) nssPKIObject_AddInstance(&c->object, instance); } c->decoding = create_decoded_pkix_cert_from_nss3cert(NULL, cc); + CERT_LockCertTempPerm(cc); cc->nssCertificate = c; + CERT_UnlockCertTempPerm(cc); return c; } |