From d8cb69a46929d2c54a809fddd0d2338e5ec14255 Mon Sep 17 00:00:00 2001 From: Jason Uh Date: Mon, 6 Jan 2014 13:20:06 -0800 Subject: [PATCH] 8007967: Infinite loop can happen in sun.security.provider.certpath.SunCertPathBuilder.depthFirstSearchForward() Reviewed-by: mullan --- .../certpath/DistributionPointFetcher.java | 58 ++++++++++++++----- .../provider/certpath/ForwardBuilder.java | 30 ++-------- .../provider/certpath/RevocationChecker.java | 18 +++--- .../provider/certpath/SunCertPathBuilder.java | 9 ++- 4 files changed, 68 insertions(+), 47 deletions(-) diff --git a/jdk/src/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java b/jdk/src/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java index bee12e43bee..ecf609b2870 100644 --- a/jdk/src/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java +++ b/jdk/src/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java @@ -75,6 +75,25 @@ public class DistributionPointFetcher { Set 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 getCRLs(X509CRLSelector selector, + boolean signFlag, + PublicKey prevKey, + X509Certificate prevCert, + String provider, + List certStores, + boolean[] reasonsMask, + Set 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 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 getCRLs(X509CRLSelector selector, X509CertImpl certImpl, DistributionPoint point, boolean[] reasonsMask, - boolean signFlag, PublicKey prevKey, String provider, - List certStores, Set trustAnchors, - Date validity) throws CertStoreException { + boolean signFlag, PublicKey prevKey, X509Certificate prevCert, + String provider, List certStores, + Set 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 trustAnchors, List 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 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); } diff --git a/jdk/src/share/classes/sun/security/provider/certpath/ForwardBuilder.java b/jdk/src/share/classes/sun/security/provider/certpath/ForwardBuilder.java index de55ef23bd4..9523e9c46bf 100644 --- a/jdk/src/share/classes/sun/security/provider/certpath/ForwardBuilder.java +++ b/jdk/src/share/classes/sun/security/provider/certpath/ForwardBuilder.java @@ -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.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.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"); } } } diff --git a/jdk/src/share/classes/sun/security/provider/certpath/RevocationChecker.java b/jdk/src/share/classes/sun/security/provider/certpath/RevocationChecker.java index bdd01c66439..19b41f6bc64 100644 --- a/jdk/src/share/classes/sun/security/provider/certpath/RevocationChecker.java +++ b/jdk/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 stackedCerts, Set 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) { diff --git a/jdk/src/share/classes/sun/security/provider/certpath/SunCertPathBuilder.java b/jdk/src/share/classes/sun/security/provider/certpath/SunCertPathBuilder.java index 9c39fb6ef4d..39507605c0f 100644 --- a/jdk/src/share/classes/sun/security/provider/certpath/SunCertPathBuilder.java +++ b/jdk/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. * - *

If successful, it returns a certification path which has succesfully + *

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