bug 171224, changes to path construction

r=nelsonb

8 files changed, 251 insertions, 92 deletions

diff --git a/security/nss/lib/certdb/certdb.c b/security/nss/lib/certdb/certdb.c
index 35ffc9e36..f3df9997f 100644
--- a/security/nss/lib/certdb/certdb.c
+++ b/security/nss/lib/certdb/certdb.c
@@ -725,6 +725,64 @@ cert_GetKeyID(CERTCertificate *cert)
 
 }
 
+static PRBool
+cert_IsRootCert(CERTCertificate *cert)
+{
+    SECStatus rv;
+    SECItem tmpitem;
+
+    /* cache the authKeyID extension, if present */
+    cert->authKeyID = CERT_FindAuthKeyIDExten(cert->arena, cert);
+
+    /* it MUST be self-issued to be a root */
+    if (cert->derIssuer.len == 0 ||
+        !SECITEM_ItemsAreEqual(&cert->derIssuer, &cert->derSubject))
+    {
+	return PR_FALSE;
+    }
+
+    /* check the authKeyID extension */
+    if (cert->authKeyID) {
+	/* authority key identifier is present */
+	if (cert->authKeyID->keyID.len > 0) {
+	    /* the keyIdentifier field is set, look for subjectKeyID */
+	    rv = CERT_FindSubjectKeyIDExten(cert, &tmpitem);
+	    if (rv == SECSuccess) {
+		PRBool match;
+		/* also present, they MUST match for it to be a root */
+		match = SECITEM_ItemsAreEqual(&cert->authKeyID->keyID,
+		                              &tmpitem);
+		PORT_Free(tmpitem.data);
+		if (!match) return PR_FALSE; /* else fall through */
+	    } else {
+		/* the subject key ID is required when AKI is present */
+		return PR_FALSE;
+	    }
+	}
+	if (cert->authKeyID->authCertIssuer) {
+	    SECItem *caName;
+	    caName = (SECItem *)CERT_GetGeneralNameByType(
+	                                  cert->authKeyID->authCertIssuer,
+	                                  certDirectoryName, PR_TRUE);
+	    if (caName) {
+		if (!SECITEM_ItemsAreEqual(&cert->derIssuer, caName)) {
+		    return PR_FALSE;
+		} /* else fall through */
+	    } /* else ??? could not get general name as directory name? */
+	}
+	if (cert->authKeyID->authCertSerialNumber.len > 0) {
+	    if (!SECITEM_ItemsAreEqual(&cert->serialNumber,
+	                         &cert->authKeyID->authCertSerialNumber)) {
+		return PR_FALSE;
+	    } /* else fall through */
+	}
+	/* all of the AKI fields that were present passed the test */
+	return PR_TRUE;
+    }
+    /* else the AKI was not present, so this is a root */
+    return PR_TRUE;
+}
+
 /*
  * take a DER certificate and decode it into a certificate structure
  */
@@ -824,6 +882,9 @@ CERT_DecodeDERCertificate(SECItem *derSignedCert, PRBool copyDER,
 	goto loser;
     }
 
+    /* determine if this is a root cert */
+    cert->isRoot = cert_IsRootCert(cert);
+
     tmpname = CERT_NameToAscii(&cert->subject);
     if ( tmpname != NULL ) {
 	cert->subjectName = PORT_ArenaStrdup(cert->arena, tmpname);
@@ -1721,7 +1782,7 @@ CERT_IsCACert(CERTCertificate *cert, unsigned int *rettype)
 
     ret = PR_FALSE;
     type = 0;
-    
+
     if ( cert->trust && (cert->trust->sslFlags|cert->trust->emailFlags|
 				cert->trust->objectSigningFlags)) {
 	trust = cert->trust;
@@ -1769,6 +1830,14 @@ CERT_IsCACert(CERTCertificate *cert, unsigned int *rettype)
 	    }
 	}
     }
+
+    /* the isRoot flag trumps all */
+    if (cert->isRoot) {
+	ret = PR_TRUE;
+	/* set only these by default, same as above */
+	type = (NS_CERT_TYPE_SSL_CA | NS_CERT_TYPE_EMAIL_CA);
+    }
+
     if ( rettype != NULL ) {
 	*rettype = type;
     }
diff --git a/security/nss/lib/certdb/certt.h b/security/nss/lib/certdb/certt.h
index 80c3d8fc8..57f22efa7 100644
--- a/security/nss/lib/certdb/certt.h
+++ b/security/nss/lib/certdb/certt.h
@@ -285,13 +285,17 @@ struct CERTCertificateStr {
      */
     CERTSubjectList *subjectList;
 
+    /* these belong in the static section, but are here to maintain
+     * the structure's integrity
+     */
+    CERTAuthKeyID * authKeyID;  /* x509v3 authority key identifier */
+    PRBool isRoot;              /* cert is the end of a chain */
+
     /* these fields are used by client GUI code to keep track of ssl sockets
      * that are blocked waiting on GUI feedback related to this cert.
      * XXX - these should be moved into some sort of application specific
      *       data structure.  They are only used by the browser right now.
      */
-    struct SECSocketNode *socketlist;
-    int socketcount;
     struct SECSocketNode *authsocketlist;
     int series; /* was int authsocketcount; record the series of the pkcs11ID */
 
diff --git a/security/nss/lib/certhigh/certhigh.c b/security/nss/lib/certhigh/certhigh.c
index 9e1278a60..f3ab3a1bf 100644
--- a/security/nss/lib/certhigh/certhigh.c
+++ b/security/nss/lib/certhigh/certhigh.c
@@ -1111,8 +1111,14 @@ loser:
 	derCert.data = (unsigned char *)stanCert->encoding.data;
 	derCert.type = siBuffer;
 	SECITEM_CopyItem(arena, &chain->certs[i], &derCert);
-	CERT_DestroyCertificate(cCert);
 	stanCert = stanChain[++i];
+	if (!stanCert && !cCert->isRoot) {
+	    /* reached the end of the chain, but the final cert is
+	     * not a root.  Don't discard it.
+	     */
+	    includeRoot = PR_TRUE;
+	}
+	CERT_DestroyCertificate(cCert);
     }
     if ( !includeRoot && len > 1) {
 	chain->len = len - 1;
diff --git a/security/nss/lib/certhigh/certvfy.c b/security/nss/lib/certhigh/certvfy.c
index a3ef430fc..221c4705a 100644
--- a/security/nss/lib/certhigh/certvfy.c
+++ b/security/nss/lib/certhigh/certvfy.c
@@ -619,7 +619,6 @@ cert_VerifyCertChain(CERTCertDBHandle *handle, CERTCertificate *cert,
     PRBool isca;
     PRBool isFortezzaV1 = PR_FALSE;
     SECStatus rv;
-    SECComparison rvCompare;
     SECStatus rvFinal = SECSuccess;
     int count;
     int currentPathLen = -1;
@@ -924,9 +923,7 @@ cert_VerifyCertChain(CERTCertDBHandle *handle, CERTCertificate *cert,
 	/* make sure that the issuer is not self signed.  If it is, then
 	 * stop here to prevent looping.
 	 */
-	rvCompare = SECITEM_CompareItem(&issuerCert->derSubject,
-				 &issuerCert->derIssuer);
-	if (rvCompare == SECEqual) {
+	if (issuerCert->isRoot) {
 	    PORT_SetError(SEC_ERROR_UNTRUSTED_ISSUER);
 	    LOG_ERROR(log, issuerCert, count+1, 0);
 	    goto loser;
diff --git a/security/nss/lib/pki/certificate.c b/security/nss/lib/pki/certificate.c
index 24bafdafd..c7c616d7e 100644
--- a/security/nss/lib/pki/certificate.c
+++ b/security/nss/lib/pki/certificate.c
@@ -280,20 +280,48 @@ nssCertificate_GetDecoding (
 static NSSCertificate **
 filter_subject_certs_for_id (
   NSSCertificate **subjectCerts, 
-  NSSItem *id
+  void *id
 )
 {
     NSSCertificate **si;
     nssDecodedCert *dcp;
     int nextOpenSlot = 0;
+    int i;
+    nssCertIDMatch matchLevel = nssCertIDMatch_Unknown;
+    nssCertIDMatch match;
 
     /* walk the subject certs */
     for (si = subjectCerts; *si; si++) {
 	dcp = nssCertificate_GetDecoding(*si);
-	if (dcp->matchIdentifier(dcp, id)) {
-	    /* this cert has the correct identifier */
+	if (!dcp) {
+	    NSSCertificate_Destroy(*si);
+	    continue;
+	}
+	match = dcp->matchIdentifier(dcp, id);
+	switch (match) {
+	case nssCertIDMatch_Yes:
+	    if (matchLevel == nssCertIDMatch_Unknown) {
+		/* we have non-definitive matches, forget them */
+		for (i = 0; i < nextOpenSlot; i++) {
+		    NSSCertificate_Destroy(subjectCerts[i]);
+		    subjectCerts[i] = NULL;
+		}
+		nextOpenSlot = 0;
+		/* only keep definitive matches from now on */
+		matchLevel = nssCertIDMatch_Yes;
+	    }
+	    /* keep the cert */
 	    subjectCerts[nextOpenSlot++] = *si;
-	} else {
+	    break;
+	case nssCertIDMatch_Unknown:
+	    if (matchLevel == nssCertIDMatch_Unknown) {
+		/* only have non-definitive matches so far, keep it */
+		subjectCerts[nextOpenSlot++] = *si;
+		break;
+	    }
+	    /* else fall through, we have a definitive match already */
+	case nssCertIDMatch_No:
+	default:
 	    NSSCertificate_Destroy(*si);
 	    *si = NULL;
 	}
@@ -302,6 +330,28 @@ filter_subject_certs_for_id (
     return subjectCerts;
 }
 
+static NSSCertificate **
+filter_certs_for_valid_issuers (
+  NSSCertificate **certs
+)
+{
+    NSSCertificate **cp;
+    nssDecodedCert *dcp;
+    int nextOpenSlot = 0;
+    int i;
+
+    for (cp = certs; *cp; cp++) {
+	dcp = nssCertificate_GetDecoding(*cp);
+	if (dcp && dcp->isValidIssuer(dcp)) {
+	    certs[nextOpenSlot++] = *cp;
+	} else {
+	    NSSCertificate_Destroy(*cp);
+	}
+    }
+    certs[nextOpenSlot] = NULL;
+    return certs;
+}
+
 static NSSCertificate *
 find_cert_issuer (
   NSSCertificate *c,
@@ -343,15 +393,15 @@ find_cert_issuer (
     certs = nssCertificateArray_Join(ccIssuers, tdIssuers);
     if (certs) {
 	nssDecodedCert *dc = NULL;
-	NSSItem *issuerID = NULL;
+	void *issuerID = NULL;
 	dc = nssCertificate_GetDecoding(c);
 	if (dc) {
 	    issuerID = dc->getIssuerIdentifier(dc);
 	}
 	if (issuerID) {
 	    certs = filter_subject_certs_for_id(certs, issuerID);
-	    nssItem_Destroy(issuerID);
-	} 
+	}
+	certs = filter_certs_for_valid_issuers(certs);
 	issuer = nssCertificateArray_FindBestCertificate(certs,
 	                                                 timeOpt,
 	                                                 usage,
@@ -378,18 +428,22 @@ nssCertificate_BuildChain (
   PRStatus *statusOpt
 )
 {
-    PRStatus status;
     NSSCertificate **rvChain;
 #ifdef NSS_3_4_CODE
     NSSCertificate *cp;
+    CERTCertificate *cCert;
 #endif
+    NSSUsage issuerUsage = *usage;
     NSSTrustDomain *td;
     nssPKIObjectCollection *collection;
+
     td = NSSCertificate_GetTrustDomain(c);
 #ifdef NSS_3_4_CODE
     if (!td) {
 	td = STAN_GetDefaultTrustDomain();
     }
+    /* bump the usage up to CA level */
+    issuerUsage.nss3lookingForCA = PR_TRUE;
 #endif
     if (statusOpt) *statusOpt = PR_SUCCESS;
     collection = nssCertificateCollection_Create(td, NULL);
@@ -401,24 +455,22 @@ nssCertificate_BuildChain (
     if (rvLimit == 1) {
 	goto finish;
     }
-    while (!nssItem_Equal(&c->subject, &c->issuer, &status)) {
+    /* XXX This breaks code for which NSS_3_4_CODE is not defined (pure
+     *     4.0 builds).  That won't affect the tip.  But be careful
+     *     when merging 4.0!!!
+     */
+    while (c != (NSSCertificate *)NULL) {
 #ifdef NSS_3_4_CODE
+	cCert = STAN_GetCERTCertificate(c);
+	if (cCert->isRoot) {
+	    /* not including the issuer of the self-signed cert, which is,
+	     * of course, itself
+	     */
+	    break;
+	}
 	cp = c;
 #endif
-	c = find_cert_issuer(c, timeOpt, usage, policiesOpt);
-#ifdef NSS_3_4_CODE
-	if (!c) {
-	    PRBool tmpca = usage->nss3lookingForCA;
-	    usage->nss3lookingForCA = PR_TRUE;
-	    c = find_cert_issuer(cp, timeOpt, usage, policiesOpt);
-	    if (!c && !usage->anyUsage) {
-		usage->anyUsage = PR_TRUE;
-		c = find_cert_issuer(cp, timeOpt, usage, policiesOpt);
-		usage->anyUsage = PR_FALSE;
-	    }
-	    usage->nss3lookingForCA = tmpca;
-	}
-#endif /* NSS_3_4_CODE */
+	c = find_cert_issuer(c, timeOpt, &issuerUsage, policiesOpt);
 	if (c) {
 	    nssPKIObjectCollection_AddObject(collection, (nssPKIObject *)c);
 	    nssCertificate_Destroy(c); /* collection has it */
diff --git a/security/nss/lib/pki/pki3hack.c b/security/nss/lib/pki/pki3hack.c
index 0b0ca3c5e..e9e9d892f 100644
--- a/security/nss/lib/pki/pki3hack.c
+++ b/security/nss/lib/pki/pki3hack.c
@@ -231,61 +231,75 @@ nss3certificate_getIdentifier(nssDecodedCert *dc)
     return rvID;
 }
 
-static NSSItem *
+static void *
 nss3certificate_getIssuerIdentifier(nssDecodedCert *dc)
 {
     CERTCertificate *c = (CERTCertificate *)dc->data;
-    CERTAuthKeyID *cAuthKeyID;
-    PRArenaPool *tmpArena = NULL;
-    NSSItem *rvID = NULL;
-    tmpArena = PORT_NewArena(512);
-    cAuthKeyID = CERT_FindAuthKeyIDExten(tmpArena, c);
-    if (cAuthKeyID == NULL) {
-	goto done;
+    return (void *)c->authKeyID;
+}
+
+static nssCertIDMatch
+nss3certificate_matchIdentifier(nssDecodedCert *dc, void *id)
+{
+    CERTCertificate *c = (CERTCertificate *)dc->data;
+    CERTAuthKeyID *authKeyID = (CERTAuthKeyID *)id;
+    SECItem skid;
+    nssCertIDMatch match = nssCertIDMatch_Unknown;
+
+    /* keyIdentifier */
+    if (authKeyID->keyID.len > 0) {
+	if (CERT_FindSubjectKeyIDExten(c, &skid) == SECSuccess) {
+	    PRBool skiEqual;
+	    skiEqual = SECITEM_ItemsAreEqual(&authKeyID->keyID, &skid);
+	    PORT_Free(skid.data);
+	    if (skiEqual) {
+		/* change the state to positive match, but keep going */
+		match = nssCertIDMatch_Yes;
+	    } else {
+		/* exit immediately on failure */
+		return nssCertIDMatch_No;
+	    }
+	} /* else fall through */
     }
-    if (cAuthKeyID->keyID.data) {
-	rvID = nssItem_Create(NULL, NULL, cAuthKeyID->keyID.len, 
-						cAuthKeyID->keyID.data);
-    } else if (cAuthKeyID->authCertIssuer) {
+
+    /* issuer/serial (treated as pair) */
+    if (authKeyID->authCertIssuer) {
 	SECItem *caName = NULL;
-	CERTIssuerAndSN issuerSN;
-	CERTCertificate *issuer = NULL;
+	SECItem *caSN = &authKeyID->authCertSerialNumber;
 
 	caName = (SECItem *)CERT_GetGeneralNameByType(
-	                                        cAuthKeyID->authCertIssuer,
+	                                        authKeyID->authCertIssuer,
 						certDirectoryName, PR_TRUE);
 	if (caName == NULL) {
-	    goto done;
+	    /* this is some kind of error, so treat it as unknown */
+	    return nssCertIDMatch_Unknown;
 	}
-	issuerSN.derIssuer.data = caName->data;
-	issuerSN.derIssuer.len = caName->len;
-	issuerSN.derIssuer.type = siBuffer;
-	issuerSN.serialNumber.data = cAuthKeyID->authCertSerialNumber.data;
-	issuerSN.serialNumber.len = cAuthKeyID->authCertSerialNumber.len;
-	issuerSN.serialNumber.type = siBuffer;
-	issuer = PK11_FindCertByIssuerAndSN(NULL, &issuerSN, NULL);
-	if (issuer) {
-	    rvID = nssItem_Create(NULL, NULL, issuer->subjectKeyID.len, 
-	    			issuer->subjectKeyID.data);
-	    CERT_DestroyCertificate(issuer);
+	if (SECITEM_ItemsAreEqual(&c->derSubject, caName) &&
+	    SECITEM_ItemsAreEqual(&c->serialNumber, caSN)) 
+	{
+	    /* change the state to positive match, but keep going */
+	    match = nssCertIDMatch_Yes;
+	} else {
+	    /* exit immediately on failure */
+	    return nssCertIDMatch_No;
 	}
     }
-done:
-    if (tmpArena) PORT_FreeArena(tmpArena, PR_FALSE);
-    return rvID;
+
+    /* If the issued cert has a keyIdentifier field with a value, but
+     * this issuer cert does not have a subjectKeyID extension, and
+     * the issuer/serial number fields of the authKeyID extension
+     * are empty, the state will be Unknown.  Otherwise it should have
+     * been set to Yes.
+     */
+    return match;
 }
 
 static PRBool
-nss3certificate_matchIdentifier(nssDecodedCert *dc, NSSItem *id)
+nss3certificate_isValidIssuer(nssDecodedCert *dc)
 {
     CERTCertificate *c = (CERTCertificate *)dc->data;
-    SECItem *subjectKeyID, authKeyID;
-    subjectKeyID = &c->subjectKeyID;
-    SECITEM_FROM_NSSITEM(&authKeyID, id);
-    if (SECITEM_CompareItem(subjectKeyID, &authKeyID) == SECEqual) {
-	return PR_TRUE;
-    }
-    return PR_FALSE;
+    unsigned int ignore;
+    return CERT_IsCACert(c, &ignore);
 }
 
 static NSSUsage *
@@ -331,6 +345,11 @@ nss3certificate_matchUsage(nssDecodedCert *dc, NSSUsage *usage)
     CERTCertificate *cc = (CERTCertificate *)dc->data;
     SECCertUsage secUsage = usage->nss3usage;
     PRBool ca = usage->nss3lookingForCA;
+
+    /* This is for NSS 3.3 functions that do not specify a usage */
+    if (usage->anyUsage) {
+	return PR_TRUE;
+    }
     secrv = CERT_KeyUsageAndTypeForCertUsage(secUsage, ca,
                                              &requiredKeyUsage,
                                              &requiredCertType);
@@ -391,6 +410,7 @@ nssDecodedPKIXCertificate_Create (
     rvDC->getIdentifier = nss3certificate_getIdentifier;
     rvDC->getIssuerIdentifier = nss3certificate_getIssuerIdentifier;
     rvDC->matchIdentifier = nss3certificate_matchIdentifier;
+    rvDC->isValidIssuer = nss3certificate_isValidIssuer;
     rvDC->getUsage = nss3certificate_getUsage;
     rvDC->isValidAtTime = nss3certificate_isValidAtTime;
     rvDC->isNewerThan = nss3certificate_isNewerThan;
@@ -413,6 +433,7 @@ create_decoded_pkix_cert_from_nss3cert (
     rvDC->getIdentifier = nss3certificate_getIdentifier;
     rvDC->getIssuerIdentifier = nss3certificate_getIssuerIdentifier;
     rvDC->matchIdentifier = nss3certificate_matchIdentifier;
+    rvDC->isValidIssuer = nss3certificate_isValidIssuer;
     rvDC->getUsage = nss3certificate_getUsage;
     rvDC->isValidAtTime = nss3certificate_isValidAtTime;
     rvDC->isNewerThan = nss3certificate_isNewerThan;
diff --git a/security/nss/lib/pki/pkibase.c b/security/nss/lib/pki/pkibase.c
index 54bf58926..162a247c4 100644
--- a/security/nss/lib/pki/pkibase.c
+++ b/security/nss/lib/pki/pkibase.c
@@ -429,6 +429,9 @@ nssCertificateArray_FindBestCertificate (
 {
     NSSCertificate *bestCert = NULL;
     NSSTime *time, sTime;
+    PRBool haveUsageMatch = PR_FALSE;
+    PRBool thisCertMatches;
+
     if (timeOpt) {
 	time = timeOpt;
     } else {
@@ -442,32 +445,31 @@ nssCertificateArray_FindBestCertificate (
 	nssDecodedCert *dc, *bestdc;
 	NSSCertificate *c = *certs;
 	dc = nssCertificate_GetDecoding(c);
+	if (!dc) continue;
+	thisCertMatches = dc->matchUsage(dc, usage);
 	if (!bestCert) {
-	    /* take the first cert with matching usage */
-#ifdef NSS_3_4_CODE
-	    if (usage->anyUsage) {
-#else
-	    if (!usage || usage->anyUsage) {
-#endif
-		bestCert = nssCertificate_AddRef(c);
-	    } else {
-		if (dc->matchUsage(dc, usage)) {
-		    bestCert = nssCertificate_AddRef(c);
-		}
-	    }
+	    /* always take the first cert, but remember whether or not
+	     * the usage matched 
+	     */
+	    bestCert = nssCertificate_AddRef(c);
+	    haveUsageMatch = thisCertMatches;
 	    continue;
 	} else {
-	    /* already have a cert for this usage, if this cert doesn't have
-	     * the correct usage, continue
-	     * if ths cert does match usage, defer to time/policies
-	     */
-#ifdef NSS_3_4_CODE
-	    if (!usage->anyUsage && !dc->matchUsage(dc, usage)) {
-#else
-	    if (PR_TRUE) {
-#endif
+	    if (haveUsageMatch && !thisCertMatches) {
+		/* if already have a cert for this usage, and if this cert 
+		 * doesn't have the correct usage, continue
+		 */
+		continue;
+	    } else if (!haveUsageMatch && thisCertMatches) {
+		/* this one does match usage, replace the other */
+		nssCertificate_Destroy(bestCert);
+		bestCert = nssCertificate_AddRef(c);
+		haveUsageMatch = PR_TRUE;
 		continue;
 	    }
+	    /* this cert match as well as any cert we've found so far, 
+	     * defer to time/policies 
+	     * */
 	}
 	bestdc = nssCertificate_GetDecoding(bestCert);
 	/* time */
diff --git a/security/nss/lib/pki/pkitm.h b/security/nss/lib/pki/pkitm.h
index d163923e6..e41879050 100644
--- a/security/nss/lib/pki/pkitm.h
+++ b/security/nss/lib/pki/pkitm.h
@@ -54,6 +54,12 @@ static const char PKITM_CVS_ID[] = "@(#) $RCSfile$ $Revision$ $Date$ $Name$";
 
 PR_BEGIN_EXTERN_C
 
+typedef enum nssCertIDMatchEnum {
+  nssCertIDMatch_Yes = 0,
+  nssCertIDMatch_No = 1,
+  nssCertIDMatch_Unknown = 2
+} nssCertIDMatch;
+
 /*
  * nssDecodedCert
  *
@@ -68,9 +74,11 @@ struct nssDecodedCertStr {
     /* returns the unique identifier for the cert */
     NSSItem *  (*getIdentifier)(nssDecodedCert *dc);
     /* returns the unique identifier for this cert's issuer */
-    NSSItem *  (*getIssuerIdentifier)(nssDecodedCert *dc);
+    void *     (*getIssuerIdentifier)(nssDecodedCert *dc);
     /* is id the identifier for this cert? */
-    PRBool     (*matchIdentifier)(nssDecodedCert *dc, NSSItem *id);
+    nssCertIDMatch (*matchIdentifier)(nssDecodedCert *dc, void *id);
+    /* is this cert a valid CA cert? */
+    PRBool     (*isValidIssuer)(nssDecodedCert *dc);
     /* returns the cert usage */
     NSSUsage * (*getUsage)(nssDecodedCert *dc);
     /* is time within the validity period of the cert? */