diff options
author | Franziskus Kiefer <franziskuskiefer@gmail.com> | 2017-02-08 10:41:02 +0100 |
---|---|---|
committer | Franziskus Kiefer <franziskuskiefer@gmail.com> | 2017-02-08 10:41:02 +0100 |
commit | 83d5a0238c71fcbbf2f81f69f589e4e9caaa1f68 (patch) | |
tree | cff11371f9ebe02efaf84832195eb5c00f1f2eca | |
parent | 4fcd4b284e329834b7a1615aab70bef05b3376e2 (diff) | |
download | nss-hg-83d5a0238c71fcbbf2f81f69f589e4e9caaa1f68.tar.gz |
Bug 1278965 - tsan race in CERTCertificate, r=wtc,ttaubert
-rw-r--r-- | lib/certdb/cert.h | 21 | ||||
-rw-r--r-- | lib/certdb/certdb.c | 50 | ||||
-rw-r--r-- | lib/certdb/certi.h | 23 | ||||
-rw-r--r-- | lib/certdb/stanpcertdb.c | 39 | ||||
-rw-r--r-- | lib/certhigh/certhigh.c | 11 | ||||
-rw-r--r-- | lib/nss/nss.def | 7 | ||||
-rw-r--r-- | lib/pk11wrap/pk11cert.c | 2 | ||||
-rw-r--r-- | lib/pki/pki3hack.c | 2 |
8 files changed, 134 insertions, 21 deletions
diff --git a/lib/certdb/cert.h b/lib/certdb/cert.h index e0af65ab0..4224da108 100644 --- a/lib/certdb/cert.h +++ b/lib/certdb/cert.h @@ -1405,24 +1405,11 @@ void CERT_SetStatusConfig(CERTCertDBHandle *handle, CERTStatusConfig *config); void CERT_LockCertRefCount(CERTCertificate *cert); /* - * Free the cert reference count lock + * Release the cert reference count lock */ void CERT_UnlockCertRefCount(CERTCertificate *cert); /* - * Acquire the cert trust lock - * There is currently one global lock for all certs, but I'm putting a cert - * arg here so that it will be easy to make it per-cert in the future if - * that turns out to be necessary. - */ -void CERT_LockCertTrust(const CERTCertificate *cert); - -/* - * Free the cert trust lock - */ -void CERT_UnlockCertTrust(const CERTCertificate *cert); - -/* * Digest the cert's subject public key using the specified algorithm. * NOTE: this digests the value of the BIT STRING subjectPublicKey (excluding * the tag, length, and number of unused bits) rather than the whole @@ -1579,6 +1566,12 @@ extern CERTRevocationFlags *CERT_AllocCERTRevocationFlags( */ extern void CERT_DestroyCERTRevocationFlags(CERTRevocationFlags *flags); +/* + * Get istemp and isperm fields from a cert in a thread safe way. + */ +extern SECStatus CERT_GetCertIsTemp(const CERTCertificate *cert, PRBool *istemp); +extern SECStatus CERT_GetCertIsPerm(const CERTCertificate *cert, PRBool *isperm); + SEC_END_PROTOS #endif /* _CERT_H_ */ diff --git a/lib/certdb/certdb.c b/lib/certdb/certdb.c index 19c99ee90..7864edc08 100644 --- a/lib/certdb/certdb.c +++ b/lib/certdb/certdb.c @@ -2865,7 +2865,18 @@ CERT_LockCertTrust(const CERTCertificate *cert) { PORT_Assert(certTrustLock != NULL); PZ_Lock(certTrustLock); - return; +} + +static PZLock *certTempPermLock = NULL; + +/* + * Acquire the cert temp/perm lock + */ +void +CERT_LockCertTempPerm(const CERTCertificate *cert) +{ + PORT_Assert(certTempPermLock != NULL); + PZ_Lock(certTempPermLock); } SECStatus @@ -2889,6 +2900,18 @@ cert_InitLocks(void) } } + if (certTempPermLock == NULL) { + certTempPermLock = PZ_NewLock(nssILockCertDB); + PORT_Assert(certTempPermLock != NULL); + if (!certTempPermLock) { + PZ_DestroyLock(certTrustLock); + PZ_DestroyLock(certRefCountLock); + certRefCountLock = NULL; + certTrustLock = NULL; + return SECFailure; + } + } + return SECSuccess; } @@ -2912,6 +2935,14 @@ cert_DestroyLocks(void) } else { rv = SECFailure; } + + PORT_Assert(certTempPermLock != NULL); + if (certTempPermLock) { + PZ_DestroyLock(certTempPermLock); + certTempPermLock = NULL; + } else { + rv = SECFailure; + } return rv; } @@ -2934,6 +2965,23 @@ CERT_UnlockCertTrust(const CERTCertificate *cert) } /* + * Free the temp/perm lock + */ +void +CERT_UnlockCertTempPerm(const CERTCertificate *cert) +{ + PORT_Assert(certTempPermLock != NULL); +#ifdef DEBUG + { + PRStatus prstat = PZ_Unlock(certTempPermLock); + PORT_Assert(prstat == PR_SUCCESS); + } +#else + (void)PZ_Unlock(certTempPermLock); +#endif +} + +/* * Get the StatusConfig data for this handle */ CERTStatusConfig * diff --git a/lib/certdb/certi.h b/lib/certdb/certi.h index 1cdf4b8fa..456f2fc4e 100644 --- a/lib/certdb/certi.h +++ b/lib/certdb/certi.h @@ -378,4 +378,27 @@ PRUint32 cert_CountDNSPatterns(CERTGeneralName* firstName); SECStatus cert_CheckLeafTrust(CERTCertificate* cert, SECCertUsage usage, unsigned int* failedFlags, PRBool* isTrusted); +/* + * Acquire the cert temp/perm lock + */ +void CERT_LockCertTempPerm(const CERTCertificate* cert); + +/* + * Release the temp/perm lock + */ +void CERT_UnlockCertTempPerm(const CERTCertificate* cert); + +/* + * Acquire the cert trust lock + * There is currently one global lock for all certs, but I'm putting a cert + * arg here so that it will be easy to make it per-cert in the future if + * that turns out to be necessary. + */ +void CERT_LockCertTrust(const CERTCertificate* cert); + +/* + * Release the cert trust lock + */ +void CERT_UnlockCertTrust(const CERTCertificate* cert); + #endif /* _CERTI_H_ */ diff --git a/lib/certdb/stanpcertdb.c b/lib/certdb/stanpcertdb.c index 3d7275df8..ac05d3c98 100644 --- a/lib/certdb/stanpcertdb.c +++ b/lib/certdb/stanpcertdb.c @@ -91,7 +91,7 @@ CERT_GetCertTrust(const CERTCertificate *cert, CERTCertTrust *trust) { SECStatus rv; CERT_LockCertTrust(cert); - if (cert->trust == NULL) { + if (!cert || cert->trust == NULL) { rv = SECFailure; } else { *trust = *cert->trust; @@ -304,8 +304,10 @@ __CERT_AddTempCertToPerm(CERTCertificate *cert, char *nickname, CERT_MapStanError(); return SECFailure; } + CERT_LockCertTempPerm(cert); cert->istemp = PR_FALSE; cert->isperm = PR_TRUE; + CERT_UnlockCertTempPerm(cert); if (!trust) { return SECSuccess; } @@ -436,8 +438,10 @@ CERT_NewTempCertificate(CERTCertDBHandle *handle, SECItem *derCert, return NULL; } + CERT_LockCertTempPerm(cc); cc->istemp = PR_TRUE; cc->isperm = PR_FALSE; + CERT_UnlockCertTempPerm(cc); return cc; loser: /* Perhaps this should be nssCertificate_Destroy(c) */ @@ -911,6 +915,7 @@ CERT_SaveSMimeProfile(CERTCertificate *cert, SECItem *emailProfile, { const char *emailAddr; SECStatus rv; + PRBool isperm = PR_FALSE; if (!cert) { return SECFailure; @@ -932,7 +937,11 @@ CERT_SaveSMimeProfile(CERTCertificate *cert, SECItem *emailProfile, } } - if (cert->slot && cert->isperm && CERT_IsUserCert(cert) && + rv = CERT_GetCertIsPerm(cert, &isperm); + if (rv != SECSuccess) { + return SECFailure; + } + if (cert->slot && isperm && CERT_IsUserCert(cert) && (!emailProfile || !emailProfile->len)) { /* Don't clobber emailProfile for user certs. */ return SECSuccess; @@ -986,6 +995,32 @@ CERT_FindSMimeProfile(CERTCertificate *cert) return rvItem; } +SECStatus +CERT_GetCertIsPerm(const CERTCertificate *cert, PRBool *isperm) +{ + if (cert == NULL) { + return SECFailure; + } + + CERT_LockCertTempPerm(cert); + *isperm = cert->isperm; + CERT_LockCertTempPerm(cert); + return SECSuccess; +} + +SECStatus +CERT_GetCertIsTemp(const CERTCertificate *cert, PRBool *istemp) +{ + if (cert == NULL) { + return SECFailure; + } + + CERT_LockCertTempPerm(cert); + *istemp = cert->istemp; + CERT_LockCertTempPerm(cert); + return SECSuccess; +} + /* * deprecated functions that are now just stubs. */ diff --git a/lib/certhigh/certhigh.c b/lib/certhigh/certhigh.c index 607fe0d3c..7ae80b193 100644 --- a/lib/certhigh/certhigh.c +++ b/lib/certhigh/certhigh.c @@ -11,6 +11,7 @@ #include "cert.h" #include "certxutl.h" +#include "certi.h" #include "nsspki.h" #include "pki.h" #include "pkit.h" @@ -872,6 +873,7 @@ cert_ImportCAChain(SECItem *certs, int numcerts, SECCertUsage certUsage, PRBool PRBool isca; char *nickname; unsigned int certtype; + PRBool istemp = PR_FALSE; handle = CERT_GetDefaultCertDB(); @@ -949,7 +951,11 @@ cert_ImportCAChain(SECItem *certs, int numcerts, SECCertUsage certUsage, PRBool } /* if the cert is temp, make it perm; otherwise we're done */ - if (cert->istemp) { + rv = CERT_GetCertIsTemp(cert, &istemp); + if (rv != SECSuccess) { + goto loser; + } + if (istemp) { /* get a default nickname for it */ nickname = CERT_MakeCANickname(cert); @@ -963,9 +969,6 @@ cert_ImportCAChain(SECItem *certs, int numcerts, SECCertUsage certUsage, PRBool rv = SECSuccess; } - CERT_DestroyCertificate(cert); - cert = NULL; - if (rv != SECSuccess) { goto loser; } diff --git a/lib/nss/nss.def b/lib/nss/nss.def index 6798103a5..e84260d7c 100644 --- a/lib/nss/nss.def +++ b/lib/nss/nss.def @@ -1104,3 +1104,10 @@ PK11_HasAttributeSet; ;+ local: ;+ *; ;+}; +;+NSS_3.31 { # NSS 3.31 release +;+ global: +CERT_GetCertIsPerm; +CERT_GetCertIsTemp; +;+ local: +;+ *; +;+}; diff --git a/lib/pk11wrap/pk11cert.c b/lib/pk11wrap/pk11cert.c index f95f4c8e9..699609947 100644 --- a/lib/pk11wrap/pk11cert.c +++ b/lib/pk11wrap/pk11cert.c @@ -974,8 +974,10 @@ PK11_ImportCert(PK11SlotInfo *slot, CERTCertificate *cert, nssCertificateStore_RemoveCertLOCKED(cc->certStore, c); nssCertificateStore_Unlock(cc->certStore, &lockTrace, &unlockTrace); c->object.cryptoContext = NULL; + CERT_LockCertTempPerm(cert); cert->istemp = PR_FALSE; cert->isperm = PR_TRUE; + CERT_UnlockCertTempPerm(cert); } /* add the new instance to the cert, force an update of the diff --git a/lib/pki/pki3hack.c b/lib/pki/pki3hack.c index 0826b7f5e..548853970 100644 --- a/lib/pki/pki3hack.c +++ b/lib/pki/pki3hack.c @@ -831,8 +831,10 @@ fill_CERTCertificateFields(NSSCertificate *c, CERTCertificate *cc, PRBool forced cc->dbhandle = c->object.trustDomain; /* subjectList ? */ /* istemp and isperm are supported in NSS 3.4 */ + 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; if (trust) { |