8007967: Infinite loop can happen in...

8007967: Infinite loop can happen in sun.security.provider.certpath.SunCertPathBuilder.depthFirstSearchForward()
Reviewed-by: mullan

src/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java

@@ -75,6 +75,25 @@ public class DistributionPointFetcher {
                                               Set<TrustAnchor> trustAnchors,
                                               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) {
@@ -101,7 +120,7 @@ public class DistributionPointFetcher {
                  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 @@ public class DistributionPointFetcher {
      */
     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 @@ public class DistributionPointFetcher {
                 // 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 @@ public class DistributionPointFetcher {
      * @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 @@ public class DistributionPointFetcher {
      */
     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 @@ public class DistributionPointFetcher {
                 // 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);
             }
 

src/share/classes/sun/security/provider/certpath/ForwardBuilder.java

 /*
- * 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.util.Debug;
 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 @@ class ForwardBuilder extends Builder {
         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");
                 }
             }
         }

src/share/classes/sun/security/provider/certpath/RevocationChecker.java

@@ -456,12 +456,13 @@ class RevocationChecker extends PKIXRevocationChecker {
                            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 @@ class RevocationChecker extends PKIXRevocationChecker {
             try {
                 if (crlDP) {
                     approvedCRLs.addAll(DistributionPointFetcher.getCRLs(
-                                        sel, signFlag, prevKey,
+                                        sel, signFlag, prevKey, prevCert,
                                         params.sigProvider(), certStores,
                                         reasonsMask, anchors, null));
                 }
@@ -825,7 +826,7 @@ class RevocationChecker extends PKIXRevocationChecker {
                 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 @@ class RevocationChecker extends PKIXRevocationChecker {
                                           + " 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 @@ class RevocationChecker extends PKIXRevocationChecker {
                     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) {

src/share/classes/sun/security/provider/certpath/SunCertPathBuilder.java

@@ -30,6 +30,7 @@ import java.security.GeneralSecurityException;
 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 @@ import sun.security.util.Debug;
  * 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 @@ public final class SunCertPathBuilder extends CertPathBuilderSpi {
                                     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;
                             }