Bug 250687

NSS Crashes or leaks Cert references if bad certs are passed up by PKCS #11 modules.
r=nelson
sr=ian
This commit is contained in:
relyea%netscape.com 2004-07-21 18:18:05 +00:00
parent 1837d24904
commit 7e82fd4194
4 changed files with 90 additions and 65 deletions

View File

@ -198,7 +198,7 @@ __CERT_AddTempCertToPerm(CERTCertificate *cert, char *nickname,
nssTrustDomain_AddCertsToCache(STAN_GetDefaultTrustDomain(), &c, 1); nssTrustDomain_AddCertsToCache(STAN_GetDefaultTrustDomain(), &c, 1);
/* reset the CERTCertificate fields */ /* reset the CERTCertificate fields */
cert->nssCertificate = NULL; cert->nssCertificate = NULL;
cert = STAN_GetCERTCertificate(c); /* will return same pointer */ cert = STAN_GetCERTCertificateOrRelease(c); /* should return same pointer */
if (!cert) { if (!cert) {
return SECFailure; return SECFailure;
} }
@ -251,7 +251,7 @@ __CERT_NewTempCertificate(CERTCertDBHandle *handle, SECItem *derCert,
PORT_SetError(SEC_ERROR_REUSED_ISSUER_AND_SERIAL); PORT_SetError(SEC_ERROR_REUSED_ISSUER_AND_SERIAL);
cc = NULL; cc = NULL;
} else { } else {
cc = STAN_GetCERTCertificate(c); cc = STAN_GetCERTCertificateOrRelease(c);
} }
return cc; 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 /* Forces a decoding of the cert in order to obtain the parts used
* below * below
*/ */
/* 'c' is not adopted here, if we fail loser frees what has been
* allocated so far for 'c' */
cc = STAN_GetCERTCertificate(c); cc = STAN_GetCERTCertificate(c);
if (!cc) { if (!cc) {
goto loser; goto loser;
@ -321,7 +323,7 @@ __CERT_NewTempCertificate(CERTCertDBHandle *handle, SECItem *derCert,
if (tempCert) { if (tempCert) {
/* and use the "official" entry */ /* and use the "official" entry */
c = tempCert; c = tempCert;
cc = STAN_GetCERTCertificate(c); cc = STAN_GetCERTCertificateOrRelease(c);
if (!cc) { if (!cc) {
return NULL; return NULL;
} }
@ -392,24 +394,12 @@ CERT_FindCertByName(CERTCertDBHandle *handle, SECItem *name)
NULL, &usage, NULL); NULL, &usage, NULL);
c = get_best_temp_or_perm(ct, cp); c = get_best_temp_or_perm(ct, cp);
if (ct) { if (ct) {
CERTCertificate *cert = STAN_GetCERTCertificate(ct); CERT_DestroyCertificate(STAN_GetCERTCertificateOrRelease(ct));
if (!cert) {
return NULL;
}
CERT_DestroyCertificate(cert);
} }
if (cp) { if (cp) {
CERTCertificate *cert = STAN_GetCERTCertificate(cp); CERT_DestroyCertificate(STAN_GetCERTCertificateOrRelease(cp));
if (!cert) {
return NULL;
}
CERT_DestroyCertificate(cert);
}
if (c) {
return STAN_GetCERTCertificate(c);
} else {
return NULL;
} }
return c ? STAN_GetCERTCertificateOrRelease(c) : NULL;
} }
CERTCertificate * CERTCertificate *
@ -458,20 +448,12 @@ CERT_FindCertByNickname(CERTCertDBHandle *handle, char *nickname)
c = get_best_temp_or_perm(ct, STAN_GetNSSCertificate(cert)); c = get_best_temp_or_perm(ct, STAN_GetNSSCertificate(cert));
CERT_DestroyCertificate(cert); CERT_DestroyCertificate(cert);
if (ct) { if (ct) {
CERTCertificate *cert2 = STAN_GetCERTCertificate(ct); CERT_DestroyCertificate(STAN_GetCERTCertificateOrRelease(ct));
if (!cert2) {
return NULL;
}
CERT_DestroyCertificate(cert2);
} }
} else { } else {
c = ct; c = ct;
} }
if (c) { return c ? STAN_GetCERTCertificateOrRelease(c) : NULL;
return STAN_GetCERTCertificate(c);
} else {
return NULL;
}
} }
CERTCertificate * CERTCertificate *
@ -488,7 +470,7 @@ CERT_FindCertByDERCert(CERTCertDBHandle *handle, SECItem *derCert)
&encoding); &encoding);
if (!c) return NULL; if (!c) return NULL;
} }
return STAN_GetCERTCertificate(c); return STAN_GetCERTCertificateOrRelease(c);
} }
CERTCertificate * CERTCertificate *
@ -520,19 +502,12 @@ CERT_FindCertByNicknameOrEmailAddr(CERTCertDBHandle *handle, char *name)
c = get_best_temp_or_perm(ct, STAN_GetNSSCertificate(cert)); c = get_best_temp_or_perm(ct, STAN_GetNSSCertificate(cert));
CERT_DestroyCertificate(cert); CERT_DestroyCertificate(cert);
if (ct) { if (ct) {
CERTCertificate *cert2 = STAN_GetCERTCertificate(ct); CERT_DestroyCertificate(STAN_GetCERTCertificateOrRelease(ct));
if (!cert2) {
return NULL;
}
CERT_DestroyCertificate(cert2);
} }
} else { } else {
c = ct; c = ct;
} }
if (c) { return c ? STAN_GetCERTCertificateOrRelease(c) : NULL;
return STAN_GetCERTCertificate(c);
}
return NULL;
} }
static void static void
@ -588,8 +563,10 @@ CERT_CreateSubjectCertList(CERTCertList *certList, CERTCertDBHandle *handle,
/* Iterate over the matching temp certs. Add them to the list */ /* Iterate over the matching temp certs. Add them to the list */
ci = tSubjectCerts; ci = tSubjectCerts;
while (ci && *ci) { 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) { if (cert) {
/* NOTE: add_to_subject_list adopts the incoming cert. */
add_to_subject_list(certList, cert, validOnly, sorttime); add_to_subject_list(certList, cert, validOnly, sorttime);
} }
ci++; ci++;
@ -597,18 +574,23 @@ CERT_CreateSubjectCertList(CERTCertList *certList, CERTCertDBHandle *handle,
/* Iterate over the matching perm certs. Add them to the list */ /* Iterate over the matching perm certs. Add them to the list */
ci = pSubjectCerts; ci = pSubjectCerts;
while (ci && *ci) { 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) { if (cert) {
/* NOTE: add_to_subject_list adopts the incoming cert. */
add_to_subject_list(certList, cert, validOnly, sorttime); add_to_subject_list(certList, cert, validOnly, sorttime);
} }
ci++; ci++;
} }
/* all the references have been adopted or freed at this point, just
* free the arrays now */
nss_ZFreeIf(tSubjectCerts); nss_ZFreeIf(tSubjectCerts);
nss_ZFreeIf(pSubjectCerts); nss_ZFreeIf(pSubjectCerts);
return certList; return certList;
loser: loser:
nss_ZFreeIf(tSubjectCerts); /* need to free the references in tSubjectCerts and pSubjectCerts! */
nss_ZFreeIf(pSubjectCerts); nssCertificateArray_Destroy(tSubjectCerts);
nssCertificateArray_Destroy(pSubjectCerts);
if (myList && certList != NULL) { if (myList && certList != NULL) {
CERT_DestroyCertList(certList); CERT_DestroyCertList(certList);
} }

View File

@ -88,6 +88,7 @@ static PRStatus convert_cert(NSSCertificate *c, void *arg)
CERTCertificate *nss3cert; CERTCertificate *nss3cert;
SECStatus secrv; SECStatus secrv;
struct nss3_cert_cbstr *nss3cb = (struct nss3_cert_cbstr *)arg; struct nss3_cert_cbstr *nss3cb = (struct nss3_cert_cbstr *)arg;
/* 'c' is not adopted. caller will free it */
nss3cert = STAN_GetCERTCertificate(c); nss3cert = STAN_GetCERTCertificate(c);
if (!nss3cert) return PR_FAILURE; if (!nss3cert) return PR_FAILURE;
secrv = (*nss3cb->callback)(nss3cert, nss3cb->arg); secrv = (*nss3cb->callback)(nss3cert, nss3cb->arg);
@ -300,7 +301,7 @@ static CERTCertificate
id.ulValueLen = c->id.size; id.ulValueLen = c->id.size;
*nickptr = pk11_buildNickname(slot, &label, privateLabel, &id); *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); nssTokenArray_Destroy(tokens);
} }
/* *must* be a valid CERTCertificate, came from cache */ CERT_DestroyCertificate(STAN_GetCERTCertificateOrRelease(certs[i]));
CERT_DestroyCertificate(STAN_GetCERTCertificate(certs[i]));
} }
nss_ZFreeIf(certs); nss_ZFreeIf(certs);
} }
@ -687,7 +687,7 @@ PK11_FindCertFromNickname(char *nickname, void *wincx)
cert = nssCertificateArray_FindBestCertificate(certs, NULL, cert = nssCertificateArray_FindBestCertificate(certs, NULL,
&usage, NULL); &usage, NULL);
if (cert) { if (cert) {
rvCert = STAN_GetCERTCertificate(cert); rvCert = STAN_GetCERTCertificateOrRelease(cert);
} }
nssCertificateArray_Destroy(certs); nssCertificateArray_Destroy(certs);
} }
@ -812,8 +812,10 @@ PK11_FindCertsFromNickname(char *nickname, void *wincx) {
PRTime now = PR_Now(); PRTime now = PR_Now();
certList = CERT_NewCertList(); certList = CERT_NewCertList();
for (i=0, c = *foundCerts; c; c = foundCerts[++i]) { 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) { if (certCert) {
/* CERT_AddCertToListSorted adopts certCert */
CERT_AddCertToListSorted(certList, certCert, CERT_AddCertToListSorted(certList, certCert,
CERT_SortCBValidity, &now); CERT_SortCBValidity, &now);
} }
@ -822,6 +824,7 @@ PK11_FindCertsFromNickname(char *nickname, void *wincx) {
CERT_DestroyCertList(certList); CERT_DestroyCertList(certList);
certList = NULL; certList = NULL;
} }
/* all the certs have been adopted or freed, free the raw array */
nss_ZFreeIf(foundCerts); nss_ZFreeIf(foundCerts);
} }
return certList; return certList;
@ -1416,6 +1419,7 @@ PK11_FindCertByIssuerAndSNOnToken(PK11SlotInfo *slot,
} }
object = NULL; /* adopted by the previous call */ object = NULL; /* adopted by the previous call */
nssTrustDomain_AddCertsToCache(td, &cert,1); nssTrustDomain_AddCertsToCache(td, &cert,1);
/* on failure, cert is freed below */
rvCert = STAN_GetCERTCertificate(cert); rvCert = STAN_GetCERTCertificate(cert);
if (!rvCert) { if (!rvCert) {
goto loser; goto loser;
@ -1759,22 +1763,34 @@ PK11_FindCertByIssuerAndSN(PK11SlotInfo **slotPtr, CERTIssuerAndSN *issuerSN,
&serial); &serial);
if (cert) { if (cert) {
SECITEM_FreeItem(derSerial, PR_TRUE); 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(), STAN_GetDefaultTrustDomain(),
&issuer, &issuer,
&serial); &serial);
if (cert) { if (!cert) {
rvCert = STAN_GetCERTCertificate(cert); break;
/* Check to see if the cert's token is still there */
if (!PK11_IsPresent(rvCert->slot)) {
CERT_DestroyCertificate(rvCert);
goto retry;
} }
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); SECITEM_FreeItem(derSerial, PR_TRUE);
return rvCert; return rvCert;
#endif #endif
@ -1912,7 +1928,7 @@ PK11_TraverseCertsForSubject(CERTCertificate *cert,
PR_FALSE,PR_TRUE,NULL); PR_FALSE,PR_TRUE,NULL);
PK11SlotListElement *le; PK11SlotListElement *le;
/* loop through all the fortezza tokens */ /* loop through all the tokens */
for (le = list->head; le; le = le->next) { for (le = list->head; le; le = le->next) {
PK11_TraverseCertsForSubjectInSlot(cert,le->slot,callback,arg); PK11_TraverseCertsForSubjectInSlot(cert,le->slot,callback,arg);
} }
@ -1997,6 +2013,9 @@ PK11_TraverseCertsForSubjectInSlot(CERTCertificate *cert, PK11SlotInfo *slot,
NSSCertificate **cp; NSSCertificate **cp;
for (cp = certs; *cp; cp++) { for (cp = certs; *cp; cp++) {
oldie = STAN_GetCERTCertificate(*cp); oldie = STAN_GetCERTCertificate(*cp);
if (!oldie) {
continue;
}
if ((*callback)(oldie, arg) != SECSuccess) { if ((*callback)(oldie, arg) != SECSuccess) {
nssrv = PR_FAILURE; nssrv = PR_FAILURE;
break; break;
@ -2094,6 +2113,9 @@ PK11_TraverseCertsForNicknameInSlot(SECItem *nickname, PK11SlotInfo *slot,
NSSCertificate **cp; NSSCertificate **cp;
for (cp = certs; *cp; cp++) { for (cp = certs; *cp; cp++) {
oldie = STAN_GetCERTCertificate(*cp); oldie = STAN_GetCERTCertificate(*cp);
if (!oldie) {
continue;
}
if ((*callback)(oldie, arg) != SECSuccess) { if ((*callback)(oldie, arg) != SECSuccess) {
nssrv = PR_FAILURE; nssrv = PR_FAILURE;
break; break;
@ -2183,6 +2205,9 @@ PK11_TraverseCertsInSlot(PK11SlotInfo *slot,
NSSCertificate **cp; NSSCertificate **cp;
for (cp = certs; *cp; cp++) { for (cp = certs; *cp; cp++) {
oldie = STAN_GetCERTCertificate(*cp); oldie = STAN_GetCERTCertificate(*cp);
if (!oldie) {
continue;
}
if ((*callback)(oldie, arg) != SECSuccess) { if ((*callback)(oldie, arg) != SECSuccess) {
nssrv = PR_FAILURE; nssrv = PR_FAILURE;
break; break;
@ -2242,10 +2267,7 @@ PK11_FindCertFromDERCertItem(PK11SlotInfo *slot, SECItem *inDerCert,
nssTokenArray_Destroy(tokens); nssTokenArray_Destroy(tokens);
} }
} }
if (c) { return c ? STAN_GetCERTCertificateOrRelease(c) : NULL;
rvCert = STAN_GetCERTCertificate(c);
}
return rvCert;
} }
/* mcgreer 3.4 -- nobody uses this, ignoring */ /* mcgreer 3.4 -- nobody uses this, ignoring */
@ -2575,6 +2597,7 @@ pk11ListCertCallback(NSSCertificate *c, void *arg)
return PR_SUCCESS; return PR_SUCCESS;
} }
/* caller still owns the reference to 'c' */
newCert = STAN_GetCERTCertificate(c); newCert = STAN_GetCERTCertificate(c);
if (!newCert) { if (!newCert) {
return PR_SUCCESS; return PR_SUCCESS;

View File

@ -35,7 +35,7 @@
* ***** END LICENSE BLOCK ***** */ * ***** END LICENSE BLOCK ***** */
#ifdef DEBUG #ifdef DEBUG
static const char CVS_ID[] = "@(#) $RCSfile: pki3hack.c,v $ $Revision: 1.81 $ $Date: 2004/05/17 20:08:37 $ $Name: $"; static const char CVS_ID[] = "@(#) $RCSfile: pki3hack.c,v $ $Revision: 1.82 $ $Date: 2004/07/21 18:18:05 $ $Name: $";
#endif /* DEBUG */ #endif /* DEBUG */
/* /*
@ -799,6 +799,23 @@ STAN_GetCERTCertificate(NSSCertificate *c)
{ {
return stan_GetCERTCertificate(c, PR_FALSE); 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 static nssTrustLevel
get_stan_trust(unsigned int t, PRBool isClientAuth) get_stan_trust(unsigned int t, PRBool isClientAuth)

View File

@ -38,7 +38,7 @@
#define PKINSS3HACK_H #define PKINSS3HACK_H
#ifdef DEBUG #ifdef DEBUG
static const char PKINSS3HACK_CVS_ID[] = "@(#) $RCSfile: pki3hack.h,v $ $Revision: 1.16 $ $Date: 2004/04/25 15:03:14 $ $Name: $"; static const char PKINSS3HACK_CVS_ID[] = "@(#) $RCSfile: pki3hack.h,v $ $Revision: 1.17 $ $Date: 2004/07/21 18:18:05 $ $Name: $";
#endif /* DEBUG */ #endif /* DEBUG */
#ifndef NSSDEVT_H #ifndef NSSDEVT_H
@ -100,6 +100,9 @@ STAN_ForceCERTCertificateUpdate(NSSCertificate *c);
NSS_EXTERN CERTCertificate * NSS_EXTERN CERTCertificate *
STAN_GetCERTCertificate(NSSCertificate *c); STAN_GetCERTCertificate(NSSCertificate *c);
NSS_EXTERN CERTCertificate *
STAN_GetCERTCertificateOrRelease(NSSCertificate *c);
NSS_EXTERN NSSCertificate * NSS_EXTERN NSSCertificate *
STAN_GetNSSCertificate(CERTCertificate *c); STAN_GetNSSCertificate(CERTCertificate *c);