8007967: Infinite loop can happen in sun.security.provider.certpath.SunCertPathBuilder.depthFirstSearchForward()
Reviewed-by: mullan
--- a/jdk/src/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java Mon Jan 06 11:48:32 2014 -0800
+++ b/jdk/src/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java Mon Jan 06 13:20:06 2014 -0800
@@ -76,6 +76,25 @@
Date validity)
throws CertStoreException
{
+ return getCRLs(selector, signFlag, prevKey, null, provider, certStores,
+ reasonsMask, trustAnchors, validity);
+ }
+
+ /**
+ * Return the X509CRLs matching this selector. The selector must be
+ * an X509CRLSelector with certificateChecking set.
+ */
+ public static Collection<X509CRL> getCRLs(X509CRLSelector selector,
+ boolean signFlag,
+ PublicKey prevKey,
+ X509Certificate prevCert,
+ String provider,
+ List<CertStore> certStores,
+ boolean[] reasonsMask,
+ Set<TrustAnchor> trustAnchors,
+ Date validity)
+ throws CertStoreException
+ {
X509Certificate cert = selector.getCertificateChecking();
if (cert == null) {
return Collections.emptySet();
@@ -101,7 +120,7 @@
t.hasNext() && !Arrays.equals(reasonsMask, ALL_REASONS); ) {
DistributionPoint point = t.next();
Collection<X509CRL> crls = getCRLs(selector, certImpl,
- point, reasonsMask, signFlag, prevKey, provider,
+ point, reasonsMask, signFlag, prevKey, prevCert, provider,
certStores, trustAnchors, validity);
results.addAll(crls);
}
@@ -125,9 +144,10 @@
*/
private static Collection<X509CRL> getCRLs(X509CRLSelector selector,
X509CertImpl certImpl, DistributionPoint point, boolean[] reasonsMask,
- boolean signFlag, PublicKey prevKey, String provider,
- List<CertStore> certStores, Set<TrustAnchor> trustAnchors,
- Date validity) throws CertStoreException {
+ boolean signFlag, PublicKey prevKey, X509Certificate prevCert,
+ String provider, List<CertStore> certStores,
+ Set<TrustAnchor> trustAnchors, Date validity)
+ throws CertStoreException {
// check for full name
GeneralNames fullName = point.getFullName();
@@ -188,8 +208,8 @@
// we check the issuer in verifyCRLs method
selector.setIssuerNames(null);
if (selector.match(crl) && verifyCRL(certImpl, point, crl,
- reasonsMask, signFlag, prevKey, provider, trustAnchors,
- certStores, validity)) {
+ reasonsMask, signFlag, prevKey, prevCert, provider,
+ trustAnchors, certStores, validity)) {
crls.add(crl);
}
} catch (IOException | CRLException e) {
@@ -284,6 +304,8 @@
* @param reasonsMask the interim reasons mask
* @param signFlag true if prevKey can be used to verify the CRL
* @param prevKey the public key that verifies the certificate's signature
+ * @param prevCert the certificate whose public key verifies
+ * {@code certImpl}'s signature
* @param provider the Signature provider to use
* @param trustAnchors a {@code Set} of {@code TrustAnchor}s
* @param certStores a {@code List} of {@code CertStore}s to be used in
@@ -294,7 +316,7 @@
*/
static boolean verifyCRL(X509CertImpl certImpl, DistributionPoint point,
X509CRL crl, boolean[] reasonsMask, boolean signFlag,
- PublicKey prevKey, String provider,
+ PublicKey prevKey, X509Certificate prevCert, String provider,
Set<TrustAnchor> trustAnchors, List<CertStore> certStores,
Date validity) throws CRLException, IOException {
@@ -591,18 +613,26 @@
// the subject criterion will be set by builder automatically.
}
- // by far, we have validated the previous certificate, we can
- // trust it during validating the CRL issuer.
- // Except the performance improvement, another benefit is to break
- // the dead loop while looking for the issuer back and forth
+ // By now, we have validated the previous certificate, so we can
+ // trust it during the validation of the CRL issuer.
+ // In addition to the performance improvement, another benefit is to
+ // break the dead loop while looking for the issuer back and forth
// between the delegated self-issued certificate and its issuer.
Set<TrustAnchor> newTrustAnchors = new HashSet<>(trustAnchors);
if (prevKey != null) {
// Add the previous certificate as a trust anchor.
- X500Principal principal = certImpl.getIssuerX500Principal();
- TrustAnchor temporary =
- new TrustAnchor(principal, prevKey, null);
+ // If prevCert is not null, we want to construct a TrustAnchor
+ // using the cert object because when the certpath for the CRL
+ // is built later, the CertSelector will make comparisons with
+ // the TrustAnchor's trustedCert member rather than its pubKey.
+ TrustAnchor temporary;
+ if (prevCert != null) {
+ temporary = new TrustAnchor(prevCert, null);
+ } else {
+ X500Principal principal = certImpl.getIssuerX500Principal();
+ temporary = new TrustAnchor(principal, prevKey, null);
+ }
newTrustAnchors.add(temporary);
}
--- a/jdk/src/share/classes/sun/security/provider/certpath/ForwardBuilder.java Mon Jan 06 11:48:32 2014 -0800
+++ b/jdk/src/share/classes/sun/security/provider/certpath/ForwardBuilder.java Mon Jan 06 13:20:06 2014 -0800
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2000, 2012, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2000, 2013, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -47,9 +47,7 @@
import sun.security.x509.AccessDescription;
import sun.security.x509.AuthorityInfoAccessExtension;
import static sun.security.x509.PKIXExtensions.*;
-import sun.security.x509.PolicyMappingsExtension;
import sun.security.x509.X500Name;
-import sun.security.x509.X509CertImpl;
import sun.security.x509.AuthorityKeyIdentifierExtension;
/**
@@ -672,32 +670,16 @@
currState.untrustedChecker.check(cert, Collections.<String>emptySet());
/*
- * check for looping - abort a loop if
- * ((we encounter the same certificate twice) AND
- * ((policyMappingInhibited = true) OR (no policy mapping
- * extensions can be found between the occurrences of the same
- * certificate)))
+ * check for looping - abort a loop if we encounter the same
+ * certificate twice
*/
if (certPathList != null) {
- boolean policyMappingFound = false;
for (X509Certificate cpListCert : certPathList) {
- X509CertImpl cpListCertImpl = X509CertImpl.toImpl(cpListCert);
- PolicyMappingsExtension policyMappingsExt
- = cpListCertImpl.getPolicyMappingsExtension();
- if (policyMappingsExt != null) {
- policyMappingFound = true;
- }
- if (debug != null) {
- debug.println("policyMappingFound = " + policyMappingFound);
- }
if (cert.equals(cpListCert)) {
- if ((buildParams.policyMappingInhibited()) ||
- (!policyMappingFound)) {
- if (debug != null) {
- debug.println("loop detected!!");
- }
- throw new CertPathValidatorException("loop detected");
+ if (debug != null) {
+ debug.println("loop detected!!");
}
+ throw new CertPathValidatorException("loop detected");
}
}
}
--- a/jdk/src/share/classes/sun/security/provider/certpath/RevocationChecker.java Mon Jan 06 11:48:32 2014 -0800
+++ b/jdk/src/share/classes/sun/security/provider/certpath/RevocationChecker.java Mon Jan 06 13:20:06 2014 -0800
@@ -456,12 +456,13 @@
PublicKey pubKey, boolean signFlag)
throws CertPathValidatorException
{
- checkCRLs(cert, pubKey, signFlag, true,
+ checkCRLs(cert, pubKey, null, signFlag, true,
stackedCerts, params.trustAnchors());
}
private void checkCRLs(X509Certificate cert, PublicKey prevKey,
- boolean signFlag, boolean allowSeparateKey,
+ X509Certificate prevCert, boolean signFlag,
+ boolean allowSeparateKey,
Set<X509Certificate> stackedCerts,
Set<TrustAnchor> anchors)
throws CertPathValidatorException
@@ -543,7 +544,7 @@
try {
if (crlDP) {
approvedCRLs.addAll(DistributionPointFetcher.getCRLs(
- sel, signFlag, prevKey,
+ sel, signFlag, prevKey, prevCert,
params.sigProvider(), certStores,
reasonsMask, anchors, null));
}
@@ -825,7 +826,7 @@
for (X509CRL crl : crls) {
if (DistributionPointFetcher.verifyCRL(
certImpl, point, crl, reasonsMask, signFlag,
- prevKey, params.sigProvider(), anchors,
+ prevKey, null, params.sigProvider(), anchors,
certStores, params.date()))
{
results.add(crl);
@@ -1043,7 +1044,7 @@
+ " index " + i + " checking "
+ cert);
}
- checkCRLs(cert, prevKey2, signFlag, true,
+ checkCRLs(cert, prevKey2, null, signFlag, true,
stackedCerts, newAnchors);
signFlag = certCanSignCrl(cert);
prevKey2 = cert.getPublicKey();
@@ -1058,13 +1059,14 @@
debug.println("RevocationChecker.buildToNewKey()" +
" got key " + cpbr.getPublicKey());
}
- // Now check revocation on the current cert using that key.
+ // Now check revocation on the current cert using that key and
+ // the corresponding certificate.
// If it doesn't check out, try to find a different key.
// And if we can't find a key, then return false.
PublicKey newKey = cpbr.getPublicKey();
try {
- checkCRLs(currCert, newKey, true, false, null,
- params.trustAnchors());
+ checkCRLs(currCert, newKey, (X509Certificate) cpList.get(0),
+ true, false, null, params.trustAnchors());
// If that passed, the cert is OK!
return;
} catch (CertPathValidatorException cpve) {
--- a/jdk/src/share/classes/sun/security/provider/certpath/SunCertPathBuilder.java Mon Jan 06 11:48:32 2014 -0800
+++ b/jdk/src/share/classes/sun/security/provider/certpath/SunCertPathBuilder.java Mon Jan 06 13:20:06 2014 -0800
@@ -30,6 +30,7 @@
import java.security.InvalidAlgorithmParameterException;
import java.security.PublicKey;
import java.security.cert.*;
+import java.security.cert.CertPathValidatorException.BasicReason;
import java.security.cert.PKIXReason;
import java.util.ArrayList;
import java.util.Collection;
@@ -49,7 +50,7 @@
* This class is able to build certification paths in either the forward
* or reverse directions.
*
- * <p> If successful, it returns a certification path which has succesfully
+ * <p> If successful, it returns a certification path which has successfully
* satisfied all the constraints and requirements specified in the
* PKIXBuilderParameters object and has been validated according to the PKIX
* path validation algorithm defined in RFC 3280.
@@ -510,6 +511,12 @@
debug.println
("SunCertPathBuilder.depthFirstSearchForward(): " +
"final verification failed: " + cpve);
+ // If the target cert itself is revoked, we
+ // cannot trust it. We can bail out here.
+ if (buildParams.targetCertConstraints().match(currCert)
+ && cpve.getReason() == BasicReason.REVOKED) {
+ throw cpve;
+ }
vertex.setThrowable(cpve);
continue vertices;
}