From 80a28612323454e32417319cd3b11e9e37b8f8ed Mon Sep 17 00:00:00 2001 From: "nelson%bolyard.com" Date: Tue, 22 Aug 2006 03:30:14 +0000 Subject: [PATCH] Fix race in CERT_NewTempCertificate. Bug 341323. r=julien,rrelyea --- security/nss/lib/certdb/stanpcertdb.c | 31 ++++------ security/nss/lib/pki/cryptocontext.c | 28 ++++++--- security/nss/lib/pki/nsspki.h | 23 +++++--- security/nss/lib/pki/pkistore.c | 81 ++++++++++++++++++++------- security/nss/lib/pki/pkistore.h | 11 ++-- 5 files changed, 116 insertions(+), 58 deletions(-) diff --git a/security/nss/lib/certdb/stanpcertdb.c b/security/nss/lib/certdb/stanpcertdb.c index 1cf14a84bd67..29857524e54e 100644 --- a/security/nss/lib/certdb/stanpcertdb.c +++ b/security/nss/lib/certdb/stanpcertdb.c @@ -229,7 +229,7 @@ __CERT_NewTempCertificate(CERTCertDBHandle *handle, SECItem *derCert, PRStatus nssrv; NSSCertificate *c; CERTCertificate *cc; - NSSCertificate *tempCert; + NSSCertificate *tempCert = NULL; nssPKIObject *pkio; NSSCryptoContext *gCC = STAN_GetDefaultCryptoContext(); NSSTrustDomain *gTD = STAN_GetDefaultTrustDomain(); @@ -311,33 +311,26 @@ __CERT_NewTempCertificate(CERTCertDBHandle *handle, SECItem *derCert, (NSSUTF8 *)cc->emailAddr, PORT_Strlen(cc->emailAddr)); } - /* this function cannot detect if the cert exists as a temp cert now, but - * didn't when CERT_NewTemp was first called. - */ - nssrv = NSSCryptoContext_ImportCertificate(gCC, c); - if (nssrv != PR_SUCCESS) { + + tempCert = NSSCryptoContext_FindOrImportCertificate(gCC, c); + if (!tempCert) { goto loser; } - /* so find the entry in the temp store */ - tempCert = NSSCryptoContext_FindCertificateByIssuerAndSerialNumber(gCC, - &c->issuer, - &c->serial); - /* destroy the copy */ + /* destroy our copy */ NSSCertificate_Destroy(c); - if (tempCert) { - /* and use the "official" entry */ - c = tempCert; - cc = STAN_GetCERTCertificateOrRelease(c); - if (!cc) { - return NULL; - } - } else { + /* and use the stored entry */ + c = tempCert; + cc = STAN_GetCERTCertificateOrRelease(c); + if (!cc) { + /* STAN_GetCERTCertificateOrRelease destroys c on failure. */ return NULL; } + cc->istemp = PR_TRUE; cc->isperm = PR_FALSE; return cc; loser: + /* Perhaps this should be nssCertificate_Destroy(c) */ nssPKIObject_Destroy(&c->object); return NULL; } diff --git a/security/nss/lib/pki/cryptocontext.c b/security/nss/lib/pki/cryptocontext.c index 663ce5dc03fa..98d6ea711bd6 100644 --- a/security/nss/lib/pki/cryptocontext.c +++ b/security/nss/lib/pki/cryptocontext.c @@ -35,7 +35,7 @@ * ***** END LICENSE BLOCK ***** */ #ifdef DEBUG -static const char CVS_ID[] = "@(#) $RCSfile: cryptocontext.c,v $ $Revision: 1.15 $ $Date: 2006/04/07 05:49:04 $"; +static const char CVS_ID[] = "@(#) $RCSfile: cryptocontext.c,v $ $Revision: 1.16 $ $Date: 2006/08/22 03:30:14 $"; #endif /* DEBUG */ #ifndef DEV_H @@ -65,6 +65,7 @@ struct NSSCryptoContextStr #endif extern const NSSError NSS_ERROR_NOT_FOUND; +extern const NSSError NSS_ERROR_INVALID_ARGUMENT; NSS_IMPLEMENT NSSCryptoContext * nssCryptoContext_Create ( @@ -142,22 +143,33 @@ NSSCryptoContext_GetTrustDomain ( return NULL; } -NSS_IMPLEMENT PRStatus -NSSCryptoContext_ImportCertificate ( + +NSS_IMPLEMENT NSSCertificate * +NSSCryptoContext_FindOrImportCertificate ( NSSCryptoContext *cc, NSSCertificate *c ) { - PRStatus nssrv; + NSSCertificate *rvCert = NULL; + PORT_Assert(cc->certStore); if (!cc->certStore) { - return PR_FAILURE; + nss_SetError(NSS_ERROR_INVALID_ARGUMENT); + return rvCert; } - nssrv = nssCertificateStore_Add(cc->certStore, c); - if (nssrv == PR_SUCCESS) { + rvCert = nssCertificateStore_FindOrAdd(cc->certStore, c); + if (rvCert == c && c->object.cryptoContext != cc) { + PORT_Assert(!c->object.cryptoContext); c->object.cryptoContext = cc; + } + if (rvCert) { + /* an NSSCertificate cannot be part of two crypto contexts + ** simultaneously. If this assertion fails, then there is + ** a serious Stan design flaw. + */ + PORT_Assert(cc == c->object.cryptoContext); } - return nssrv; + return rvCert; } NSS_IMPLEMENT NSSCertificate * diff --git a/security/nss/lib/pki/nsspki.h b/security/nss/lib/pki/nsspki.h index b7b80c8a2f05..b3c5cd6da5a3 100644 --- a/security/nss/lib/pki/nsspki.h +++ b/security/nss/lib/pki/nsspki.h @@ -38,7 +38,7 @@ #define NSSPKI_H #ifdef DEBUG -static const char NSSPKI_CVS_ID[] = "@(#) $RCSfile: nsspki.h,v $ $Revision: 1.10 $ $Date: 2005/01/20 02:25:49 $"; +static const char NSSPKI_CVS_ID[] = "@(#) $RCSfile: nsspki.h,v $ $Revision: 1.11 $ $Date: 2006/08/22 03:30:14 $"; #endif /* DEBUG */ /* @@ -2235,15 +2235,24 @@ NSSCryptoContext_GetTrustDomain /* Importing things */ /* - * NSSCryptoContext_ImportCertificate + * NSSCryptoContext_FindOrImportCertificate * - * If there's not a "distinguished certificate" for this context, this - * sets the specified one to be it. + * If the certificate store already contains this DER cert, return the + * address of the matching NSSCertificate that is already in the store, + * and bump its reference count. + * + * If this DER cert is NOT already in the store, then add the new + * NSSCertificate to the store and bump its reference count, + * then return its address. + * + * if this DER cert is not in the store and cannot be added to it, + * return NULL; + * + * Record the associated crypto context in the certificate. */ -NSS_EXTERN PRStatus -NSSCryptoContext_ImportCertificate -( +NSS_EXTERN NSSCertificate * +NSSCryptoContext_FindOrImportCertificate ( NSSCryptoContext *cc, NSSCertificate *c ); diff --git a/security/nss/lib/pki/pkistore.c b/security/nss/lib/pki/pkistore.c index 47106e17c99d..e6ca5a99494d 100644 --- a/security/nss/lib/pki/pkistore.c +++ b/security/nss/lib/pki/pkistore.c @@ -35,7 +35,7 @@ * ***** END LICENSE BLOCK ***** */ #ifdef DEBUG -static const char CVS_ID[] = "@(#) $RCSfile: pkistore.c,v $ $Revision: 1.27 $ $Date: 2006/04/07 05:49:04 $"; +static const char CVS_ID[] = "@(#) $RCSfile: pkistore.c,v $ $Revision: 1.28 $ $Date: 2006/08/22 03:30:14 $"; #endif /* DEBUG */ #ifndef PKIM_H @@ -89,6 +89,14 @@ struct certificate_hash_entry_str nssSMIMEProfile *profile; }; +/* forward static declarations */ +static NSSCertificate * +nssCertStore_FindCertByIssuerAndSerialNumberLocked ( + nssCertificateStore *store, + NSSDER *issuer, + NSSDER *serial +); + NSS_IMPLEMENT nssCertificateStore * nssCertificateStore_Create ( NSSArena *arenaOpt @@ -225,29 +233,46 @@ remove_certificate_entry ( NSSCertificate *cert ); -NSS_IMPLEMENT PRStatus -nssCertificateStore_Add ( +/* Caller must hold store->lock */ +static PRStatus +nssCertificateStore_AddLocked ( nssCertificateStore *store, NSSCertificate *cert ) { - PRStatus nssrv; - PZ_Lock(store->lock); - if (nssHash_Exists(store->issuer_and_serial, cert)) { - PZ_Unlock(store->lock); - return PR_SUCCESS; - } - nssrv = add_certificate_entry(store, cert); + PRStatus nssrv = add_certificate_entry(store, cert); if (nssrv == PR_SUCCESS) { nssrv = add_subject_entry(store, cert); if (nssrv == PR_FAILURE) { remove_certificate_entry(store, cert); } } - PZ_Unlock(store->lock); return nssrv; } + +NSS_IMPLEMENT NSSCertificate * +nssCertificateStore_FindOrAdd ( + nssCertificateStore *store, + NSSCertificate *c +) +{ + PRStatus nssrv; + NSSCertificate *rvCert = NULL; + + PZ_Lock(store->lock); + rvCert = nssCertStore_FindCertByIssuerAndSerialNumberLocked( + store, &c->issuer, &c->serial); + if (!rvCert) { + nssrv = nssCertificateStore_AddLocked(store, c); + if (PR_SUCCESS == nssrv) { + rvCert = nssCertificate_AddRef(c); + } + } + PZ_Unlock(store->lock); + return rvCert; +} + static void remove_certificate_entry ( nssCertificateStore *store, @@ -524,6 +549,28 @@ nssCertificateStore_FindCertificatesByEmail ( return rvArray; } +/* Caller holds store->lock */ +static NSSCertificate * +nssCertStore_FindCertByIssuerAndSerialNumberLocked ( + nssCertificateStore *store, + NSSDER *issuer, + NSSDER *serial +) +{ + certificate_hash_entry *entry; + NSSCertificate *rvCert = NULL; + NSSCertificate index; + + index.issuer = *issuer; + index.serial = *serial; + entry = (certificate_hash_entry *) + nssHash_Lookup(store->issuer_and_serial, &index); + if (entry) { + rvCert = nssCertificate_AddRef(entry->cert); + } + return rvCert; +} + NSS_IMPLEMENT NSSCertificate * nssCertificateStore_FindCertificateByIssuerAndSerialNumber ( nssCertificateStore *store, @@ -531,17 +578,11 @@ nssCertificateStore_FindCertificateByIssuerAndSerialNumber ( NSSDER *serial ) { - certificate_hash_entry *entry; - NSSCertificate index; NSSCertificate *rvCert = NULL; - index.issuer = *issuer; - index.serial = *serial; + PZ_Lock(store->lock); - entry = (certificate_hash_entry *) - nssHash_Lookup(store->issuer_and_serial, &index); - if (entry) { - rvCert = nssCertificate_AddRef(entry->cert); - } + rvCert = nssCertStore_FindCertByIssuerAndSerialNumberLocked ( + store, issuer, serial); PZ_Unlock(store->lock); return rvCert; } diff --git a/security/nss/lib/pki/pkistore.h b/security/nss/lib/pki/pkistore.h index d8e737d6b7ab..9a9b73192484 100644 --- a/security/nss/lib/pki/pkistore.h +++ b/security/nss/lib/pki/pkistore.h @@ -38,7 +38,7 @@ #define PKISTORE_H #ifdef DEBUG -static const char PKISTORE_CVS_ID[] = "@(#) $RCSfile: pkistore.h,v $ $Revision: 1.9 $ $Date: 2006/04/19 19:04:23 $"; +static const char PKISTORE_CVS_ID[] = "@(#) $RCSfile: pkistore.h,v $ $Revision: 1.10 $ $Date: 2006/08/22 03:30:14 $"; #endif /* DEBUG */ #ifndef NSSPKIT_H @@ -81,11 +81,14 @@ nssCertificateStore_Destroy nssCertificateStore *store ); -NSS_EXTERN PRStatus -nssCertificateStore_Add +/* Atomic Find cert in store, or add this cert to the store. +** Ref counts properly maintained. +*/ +NSS_EXTERN NSSCertificate * +nssCertificateStore_FindOrAdd ( nssCertificateStore *store, - NSSCertificate *cert + NSSCertificate *c ); NSS_EXTERN void