Bug 250687

   NSS Crashes or leaks Cert references if bad certs are passed up by PKCS #11 modules.
r=nelson
sr=ian

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);