From 97899cf8ea2cb6eff9419b374e20c0c2cee5e261 Mon Sep 17 00:00:00 2001 From: "relyea%netscape.com" Date: Wed, 21 Jul 2004 18:18:05 +0000 Subject: Bug 250687 NSS Crashes or leaks Cert references if bad certs are passed up by PKCS #11 modules. r=nelson sr=ian --- security/nss/lib/certdb/stanpcertdb.c | 66 +++++++++++++---------------------- security/nss/lib/pk11wrap/pk11cert.c | 65 +++++++++++++++++++++++----------- security/nss/lib/pki/pki3hack.c | 17 +++++++++ security/nss/lib/pki/pki3hack.h | 3 ++ 4 files changed, 88 insertions(+), 63 deletions(-) diff --git a/security/nss/lib/certdb/stanpcertdb.c b/security/nss/lib/certdb/stanpcertdb.c index 14085dab9..7c6185814 100644 --- a/security/nss/lib/certdb/stanpcertdb.c +++ b/security/nss/lib/certdb/stanpcertdb.c @@ -198,7 +198,7 @@ __CERT_AddTempCertToPerm(CERTCertificate *cert, char *nickname, nssTrustDomain_AddCertsToCache(STAN_GetDefaultTrustDomain(), &c, 1); /* reset the CERTCertificate fields */ cert->nssCertificate = NULL; - cert = STAN_GetCERTCertificate(c); /* will return same pointer */ + cert = STAN_GetCERTCertificateOrRelease(c); /* should return same pointer */ if (!cert) { return SECFailure; } @@ -251,7 +251,7 @@ __CERT_NewTempCertificate(CERTCertDBHandle *handle, SECItem *derCert, PORT_SetError(SEC_ERROR_REUSED_ISSUER_AND_SERIAL); cc = NULL; } else { - cc = STAN_GetCERTCertificate(c); + cc = STAN_GetCERTCertificateOrRelease(c); } return cc; } @@ -275,6 +275,8 @@ __CERT_NewTempCertificate(CERTCertDBHandle *handle, SECItem *derCert, /* Forces a decoding of the cert in order to obtain the parts used * below */ + /* 'c' is not adopted here, if we fail loser frees what has been + * allocated so far for 'c' */ cc = STAN_GetCERTCertificate(c); if (!cc) { goto loser; @@ -321,7 +323,7 @@ __CERT_NewTempCertificate(CERTCertDBHandle *handle, SECItem *derCert, if (tempCert) { /* and use the "official" entry */ c = tempCert; - cc = STAN_GetCERTCertificate(c); + cc = STAN_GetCERTCertificateOrRelease(c); if (!cc) { return NULL; } @@ -392,24 +394,12 @@ CERT_FindCertByName(CERTCertDBHandle *handle, SECItem *name) NULL, &usage, NULL); c = get_best_temp_or_perm(ct, cp); if (ct) { - CERTCertificate *cert = STAN_GetCERTCertificate(ct); - if (!cert) { - return NULL; - } - CERT_DestroyCertificate(cert); + CERT_DestroyCertificate(STAN_GetCERTCertificateOrRelease(ct)); } if (cp) { - CERTCertificate *cert = STAN_GetCERTCertificate(cp); - if (!cert) { - return NULL; - } - CERT_DestroyCertificate(cert); - } - if (c) { - return STAN_GetCERTCertificate(c); - } else { - return NULL; + CERT_DestroyCertificate(STAN_GetCERTCertificateOrRelease(cp)); } + return c ? STAN_GetCERTCertificateOrRelease(c) : NULL; } CERTCertificate * @@ -458,20 +448,12 @@ CERT_FindCertByNickname(CERTCertDBHandle *handle, char *nickname) c = get_best_temp_or_perm(ct, STAN_GetNSSCertificate(cert)); CERT_DestroyCertificate(cert); if (ct) { - CERTCertificate *cert2 = STAN_GetCERTCertificate(ct); - if (!cert2) { - return NULL; - } - CERT_DestroyCertificate(cert2); + CERT_DestroyCertificate(STAN_GetCERTCertificateOrRelease(ct)); } } else { c = ct; } - if (c) { - return STAN_GetCERTCertificate(c); - } else { - return NULL; - } + return c ? STAN_GetCERTCertificateOrRelease(c) : NULL; } CERTCertificate * @@ -488,7 +470,7 @@ CERT_FindCertByDERCert(CERTCertDBHandle *handle, SECItem *derCert) &encoding); if (!c) return NULL; } - return STAN_GetCERTCertificate(c); + return STAN_GetCERTCertificateOrRelease(c); } CERTCertificate * @@ -520,19 +502,12 @@ CERT_FindCertByNicknameOrEmailAddr(CERTCertDBHandle *handle, char *name) c = get_best_temp_or_perm(ct, STAN_GetNSSCertificate(cert)); CERT_DestroyCertificate(cert); if (ct) { - CERTCertificate *cert2 = STAN_GetCERTCertificate(ct); - if (!cert2) { - return NULL; - } - CERT_DestroyCertificate(cert2); + CERT_DestroyCertificate(STAN_GetCERTCertificateOrRelease(ct)); } } else { c = ct; } - if (c) { - return STAN_GetCERTCertificate(c); - } - return NULL; + return c ? STAN_GetCERTCertificateOrRelease(c) : NULL; } static void @@ -588,8 +563,10 @@ CERT_CreateSubjectCertList(CERTCertList *certList, CERTCertDBHandle *handle, /* Iterate over the matching temp certs. Add them to the list */ ci = tSubjectCerts; while (ci && *ci) { - cert = STAN_GetCERTCertificate(*ci); + cert = STAN_GetCERTCertificateOrRelease(*ci); + /* *ci may be invalid at this point, don't reference it again */ if (cert) { + /* NOTE: add_to_subject_list adopts the incoming cert. */ add_to_subject_list(certList, cert, validOnly, sorttime); } ci++; @@ -597,18 +574,23 @@ CERT_CreateSubjectCertList(CERTCertList *certList, CERTCertDBHandle *handle, /* Iterate over the matching perm certs. Add them to the list */ ci = pSubjectCerts; while (ci && *ci) { - cert = STAN_GetCERTCertificate(*ci); + cert = STAN_GetCERTCertificateOrRelease(*ci); + /* *ci may be invalid at this point, don't reference it again */ if (cert) { + /* NOTE: add_to_subject_list adopts the incoming cert. */ add_to_subject_list(certList, cert, validOnly, sorttime); } ci++; } + /* all the references have been adopted or freed at this point, just + * free the arrays now */ nss_ZFreeIf(tSubjectCerts); nss_ZFreeIf(pSubjectCerts); return certList; loser: - nss_ZFreeIf(tSubjectCerts); - nss_ZFreeIf(pSubjectCerts); + /* need to free the references in tSubjectCerts and pSubjectCerts! */ + nssCertificateArray_Destroy(tSubjectCerts); + nssCertificateArray_Destroy(pSubjectCerts); if (myList && certList != NULL) { CERT_DestroyCertList(certList); } diff --git a/security/nss/lib/pk11wrap/pk11cert.c b/security/nss/lib/pk11wrap/pk11cert.c index d5eaebba8..3f9ee42f3 100644 --- a/security/nss/lib/pk11wrap/pk11cert.c +++ b/security/nss/lib/pk11wrap/pk11cert.c @@ -88,6 +88,7 @@ static PRStatus convert_cert(NSSCertificate *c, void *arg) CERTCertificate *nss3cert; SECStatus secrv; struct nss3_cert_cbstr *nss3cb = (struct nss3_cert_cbstr *)arg; + /* 'c' is not adopted. caller will free it */ nss3cert = STAN_GetCERTCertificate(c); if (!nss3cert) return PR_FAILURE; secrv = (*nss3cb->callback)(nss3cert, nss3cb->arg); @@ -300,7 +301,7 @@ static CERTCertificate id.ulValueLen = c->id.size; *nickptr = pk11_buildNickname(slot, &label, privateLabel, &id); } - return STAN_GetCERTCertificate(c); + return STAN_GetCERTCertificateOrRelease(c); } /* @@ -575,8 +576,7 @@ transfer_token_certs_to_collection(nssList *certList, NSSToken *token, } nssTokenArray_Destroy(tokens); } - /* *must* be a valid CERTCertificate, came from cache */ - CERT_DestroyCertificate(STAN_GetCERTCertificate(certs[i])); + CERT_DestroyCertificate(STAN_GetCERTCertificateOrRelease(certs[i])); } nss_ZFreeIf(certs); } @@ -687,7 +687,7 @@ PK11_FindCertFromNickname(char *nickname, void *wincx) cert = nssCertificateArray_FindBestCertificate(certs, NULL, &usage, NULL); if (cert) { - rvCert = STAN_GetCERTCertificate(cert); + rvCert = STAN_GetCERTCertificateOrRelease(cert); } nssCertificateArray_Destroy(certs); } @@ -812,8 +812,10 @@ PK11_FindCertsFromNickname(char *nickname, void *wincx) { PRTime now = PR_Now(); certList = CERT_NewCertList(); for (i=0, c = *foundCerts; c; c = foundCerts[++i]) { - CERTCertificate *certCert = STAN_GetCERTCertificate(c); + CERTCertificate *certCert = STAN_GetCERTCertificateOrRelease(c); + /* c may be invalid after this, don't reference it */ if (certCert) { + /* CERT_AddCertToListSorted adopts certCert */ CERT_AddCertToListSorted(certList, certCert, CERT_SortCBValidity, &now); } @@ -822,6 +824,7 @@ PK11_FindCertsFromNickname(char *nickname, void *wincx) { CERT_DestroyCertList(certList); certList = NULL; } + /* all the certs have been adopted or freed, free the raw array */ nss_ZFreeIf(foundCerts); } return certList; @@ -1416,6 +1419,7 @@ PK11_FindCertByIssuerAndSNOnToken(PK11SlotInfo *slot, } object = NULL; /* adopted by the previous call */ nssTrustDomain_AddCertsToCache(td, &cert,1); + /* on failure, cert is freed below */ rvCert = STAN_GetCERTCertificate(cert); if (!rvCert) { goto loser; @@ -1759,22 +1763,34 @@ PK11_FindCertByIssuerAndSN(PK11SlotInfo **slotPtr, CERTIssuerAndSN *issuerSN, &serial); if (cert) { SECITEM_FreeItem(derSerial, PR_TRUE); - return STAN_GetCERTCertificate(cert); + return STAN_GetCERTCertificateOrRelease(cert); } -retry: - cert = NSSTrustDomain_FindCertificateByIssuerAndSerialNumber( + + do { + /* free the old cert on retry. Associated slot was not present */ + if (rvCert) { + CERT_DestroyCertificate(rvCert); + rvCert = NULL; + } + + cert = NSSTrustDomain_FindCertificateByIssuerAndSerialNumber( STAN_GetDefaultTrustDomain(), &issuer, &serial); - if (cert) { - rvCert = STAN_GetCERTCertificate(cert); - /* Check to see if the cert's token is still there */ - if (!PK11_IsPresent(rvCert->slot)) { - CERT_DestroyCertificate(rvCert); - goto retry; + if (!cert) { + break; } - if (slotPtr) *slotPtr = PK11_ReferenceSlot(rvCert->slot); - } + + rvCert = STAN_GetCERTCertificateOrRelease(cert); + if (rvCert == NULL) { + break; + } + + /* Check to see if the cert's token is still there */ + } while (!PK11_IsPresent(rvCert->slot)); + + if (rvCert && slotPtr) *slotPtr = PK11_ReferenceSlot(rvCert->slot); + SECITEM_FreeItem(derSerial, PR_TRUE); return rvCert; #endif @@ -1912,7 +1928,7 @@ PK11_TraverseCertsForSubject(CERTCertificate *cert, PR_FALSE,PR_TRUE,NULL); PK11SlotListElement *le; - /* loop through all the fortezza tokens */ + /* loop through all the tokens */ for (le = list->head; le; le = le->next) { PK11_TraverseCertsForSubjectInSlot(cert,le->slot,callback,arg); } @@ -1997,6 +2013,9 @@ PK11_TraverseCertsForSubjectInSlot(CERTCertificate *cert, PK11SlotInfo *slot, NSSCertificate **cp; for (cp = certs; *cp; cp++) { oldie = STAN_GetCERTCertificate(*cp); + if (!oldie) { + continue; + } if ((*callback)(oldie, arg) != SECSuccess) { nssrv = PR_FAILURE; break; @@ -2094,6 +2113,9 @@ PK11_TraverseCertsForNicknameInSlot(SECItem *nickname, PK11SlotInfo *slot, NSSCertificate **cp; for (cp = certs; *cp; cp++) { oldie = STAN_GetCERTCertificate(*cp); + if (!oldie) { + continue; + } if ((*callback)(oldie, arg) != SECSuccess) { nssrv = PR_FAILURE; break; @@ -2183,6 +2205,9 @@ PK11_TraverseCertsInSlot(PK11SlotInfo *slot, NSSCertificate **cp; for (cp = certs; *cp; cp++) { oldie = STAN_GetCERTCertificate(*cp); + if (!oldie) { + continue; + } if ((*callback)(oldie, arg) != SECSuccess) { nssrv = PR_FAILURE; break; @@ -2242,10 +2267,7 @@ PK11_FindCertFromDERCertItem(PK11SlotInfo *slot, SECItem *inDerCert, nssTokenArray_Destroy(tokens); } } - if (c) { - rvCert = STAN_GetCERTCertificate(c); - } - return rvCert; + return c ? STAN_GetCERTCertificateOrRelease(c) : NULL; } /* mcgreer 3.4 -- nobody uses this, ignoring */ @@ -2575,6 +2597,7 @@ pk11ListCertCallback(NSSCertificate *c, void *arg) return PR_SUCCESS; } + /* caller still owns the reference to 'c' */ newCert = STAN_GetCERTCertificate(c); if (!newCert) { return PR_SUCCESS; diff --git a/security/nss/lib/pki/pki3hack.c b/security/nss/lib/pki/pki3hack.c index 50946c527..1ea7e2167 100644 --- a/security/nss/lib/pki/pki3hack.c +++ b/security/nss/lib/pki/pki3hack.c @@ -799,6 +799,23 @@ STAN_GetCERTCertificate(NSSCertificate *c) { return stan_GetCERTCertificate(c, PR_FALSE); } +/* + * many callers of STAN_GetCERTCertificate() intend that + * the CERTCertificate returned inherits the reference to the + * NSSCertificate. For these callers it's convenient to have + * this function 'own' the reference and either return a valid + * CERTCertificate structure which inherits the reference or + * destroy the reference to NSSCertificate and returns NULL. + */ +NSS_IMPLEMENT CERTCertificate * +STAN_GetCERTCertificateOrRelease(NSSCertificate *c) +{ + CERTCertificate *nss3cert = stan_GetCERTCertificate(c, PR_FALSE); + if (!nss3cert) { + nssCertificate_Destroy(c); + } + return nss3cert; +} static nssTrustLevel get_stan_trust(unsigned int t, PRBool isClientAuth) diff --git a/security/nss/lib/pki/pki3hack.h b/security/nss/lib/pki/pki3hack.h index 0d55816f1..1e8fcc9f8 100644 --- a/security/nss/lib/pki/pki3hack.h +++ b/security/nss/lib/pki/pki3hack.h @@ -100,6 +100,9 @@ STAN_ForceCERTCertificateUpdate(NSSCertificate *c); NSS_EXTERN CERTCertificate * STAN_GetCERTCertificate(NSSCertificate *c); +NSS_EXTERN CERTCertificate * +STAN_GetCERTCertificateOrRelease(NSSCertificate *c); + NSS_EXTERN NSSCertificate * STAN_GetNSSCertificate(CERTCertificate *c); -- cgit v1.2.1