8243585: AlgorithmChecker::check throws confusing exception when it rejects the signer key

Reviewed-by: ascarpino

Author: seanjmullan

src/java.base/share/classes/sun/security/provider/certpath/AlgorithmChecker.java

@@ -50,8 +50,8 @@ public class CertPathConstraintsParameters implements ConstraintsParameters {
     private final Date date;
     // The variant or usage of this certificate
     private final String variant;
-    // The certificate being checked (may be null if a CRL or OCSPResponse is
-    // being checked)
+    // The certificate being checked (may be null if a raw public key, a CRL
+    // or an OCSPResponse is being checked)
     private final X509Certificate cert;
 
     public CertPathConstraintsParameters(X509Certificate cert,
@@ -60,8 +60,8 @@ public CertPathConstraintsParameters(X509Certificate cert,
     }
 
     public CertPathConstraintsParameters(Key key, String variant,
-            TrustAnchor anchor) {
-        this(key, variant, anchor, null, null);
+            TrustAnchor anchor, Date date) {
+        this(key, variant, anchor, date, null);
     }
 
     private CertPathConstraintsParameters(Key key, String variant,

src/java.base/share/classes/sun/security/provider/certpath/CertPathConstraintsParameters.java

@@ -176,8 +176,8 @@ private static PKIXCertPathValidatorResult validate(TrustAnchor anchor,
         List<PKIXCertPathChecker> certPathCheckers = new ArrayList<>();
         // add standard checkers that we will be using
         certPathCheckers.add(untrustedChecker);
-        certPathCheckers.add(new AlgorithmChecker(anchor, null,
-                params.timestamp(), params.variant()));
+        certPathCheckers.add(new AlgorithmChecker(anchor, params.timestamp(),
+                params.variant()));
         certPathCheckers.add(new KeyChecker(certPathLen,
                                             params.targetCertConstraints()));
         certPathCheckers.add(new ConstraintsChecker(certPathLen));

src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2021, 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
@@ -26,6 +26,8 @@
 package sun.security.util;
 
 import java.util.HashSet;
+import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Set;
 import java.util.Arrays;
 import java.util.Collection;
@@ -40,6 +42,14 @@ public class AlgorithmDecomposer {
     private static final Pattern PATTERN =
             Pattern.compile("with|and|(?<!padd)in", Pattern.CASE_INSENSITIVE);
 
+    // A map of standard message digest algorithm names to decomposed names
+    // so that a constraint can match for example, "SHA-1" and also
+    // "SHA1withRSA".
+    private static final Map<String, String> DECOMPOSED_DIGEST_NAMES =
+        Map.of("SHA-1", "SHA1", "SHA-224", "SHA224", "SHA-256", "SHA256",
+               "SHA-384", "SHA384", "SHA-512", "SHA512", "SHA-512/224",
+               "SHA512/224", "SHA-512/256", "SHA512/256");
+
     private static Set<String> decomposeImpl(String algorithm) {
         Set<String> elements = new HashSet<>();
 
@@ -93,44 +103,19 @@ public Set<String> decompose(String algorithm) {
         // signature algorithm "SHA256withRSA". So we need to check both
         // "SHA-256" and "SHA256" to make the right constraint checking.
 
-        // handle special name: SHA-1 and SHA1
-        if (elements.contains("SHA1") && !elements.contains("SHA-1")) {
-            elements.add("SHA-1");
-        }
-        if (elements.contains("SHA-1") && !elements.contains("SHA1")) {
-            elements.add("SHA1");
+        // no need to check further if algorithm doesn't contain "SHA"
+        if (!algorithm.contains("SHA")) {
+            return elements;
         }
 
-        // handle special name: SHA-224 and SHA224
-        if (elements.contains("SHA224") && !elements.contains("SHA-224")) {
-            elements.add("SHA-224");
-        }
-        if (elements.contains("SHA-224") && !elements.contains("SHA224")) {
-            elements.add("SHA224");
-        }
-
-        // handle special name: SHA-256 and SHA256
-        if (elements.contains("SHA256") && !elements.contains("SHA-256")) {
-            elements.add("SHA-256");
-        }
-        if (elements.contains("SHA-256") && !elements.contains("SHA256")) {
-            elements.add("SHA256");
-        }
-
-        // handle special name: SHA-384 and SHA384
-        if (elements.contains("SHA384") && !elements.contains("SHA-384")) {
-            elements.add("SHA-384");
-        }
-        if (elements.contains("SHA-384") && !elements.contains("SHA384")) {
-            elements.add("SHA384");
-        }
-
-        // handle special name: SHA-512 and SHA512
-        if (elements.contains("SHA512") && !elements.contains("SHA-512")) {
-            elements.add("SHA-512");
-        }
-        if (elements.contains("SHA-512") && !elements.contains("SHA512")) {
-            elements.add("SHA512");
+        for (Map.Entry<String, String> e : DECOMPOSED_DIGEST_NAMES.entrySet()) {
+            if (elements.contains(e.getValue()) &&
+                    !elements.contains(e.getKey())) {
+                elements.add(e.getKey());
+            } else if (elements.contains(e.getKey()) &&
+                    !elements.contains(e.getValue())) {
+                elements.add(e.getValue());
+            }
         }
 
         return elements;
@@ -153,40 +138,44 @@ public static Collection<String> getAliases(String algorithm) {
         return Arrays.asList(aliases);
     }
 
-    private static void hasLoop(Set<String> elements, String find, String replace) {
-        if (elements.contains(find)) {
-            if (!elements.contains(replace)) {
-                elements.add(replace);
-            }
-            elements.remove(find);
-        }
-    }
-
-    /*
-     * This decomposes a standard name into sub-elements with a consistent
-     * message digest algorithm name to avoid overly complicated checking.
+    /**
+     * Decomposes a standard algorithm name into sub-elements and uses a
+     * consistent message digest algorithm name to avoid overly complicated
+     * checking.
      */
-    public static Set<String> decomposeOneHash(String algorithm) {
+    static Set<String> decomposeName(String algorithm) {
         if (algorithm == null || algorithm.isEmpty()) {
             return new HashSet<>();
         }
 
         Set<String> elements = decomposeImpl(algorithm);
 
-        hasLoop(elements, "SHA-1", "SHA1");
-        hasLoop(elements, "SHA-224", "SHA224");
-        hasLoop(elements, "SHA-256", "SHA256");
-        hasLoop(elements, "SHA-384", "SHA384");
-        hasLoop(elements, "SHA-512", "SHA512");
+        // no need to check further if algorithm doesn't contain "SHA"
+        if (!algorithm.contains("SHA")) {
+            return elements;
+        }
+
+        for (Map.Entry<String, String> e : DECOMPOSED_DIGEST_NAMES.entrySet()) {
+            if (elements.contains(e.getKey())) {
+                if (!elements.contains(e.getValue())) {
+                    elements.add(e.getValue());
+                }
+                elements.remove(e.getKey());
+            }
+        }
 
         return elements;
     }
 
-    /*
-     * The provided message digest algorithm name will return a consistent
-     * naming scheme.
+    /**
+     * Decomposes a standard message digest algorithm name into a consistent
+     * name for matching purposes.
+     *
+     * @param algorithm the name to be decomposed
+     * @return the decomposed name, or the passed in algorithm name if
+     *     it is not a digest algorithm or does not need to be decomposed
      */
-    public static String hashName(String algorithm) {
-        return algorithm.replace("-", "");
+    static String decomposeDigestName(String algorithm) {
+        return DECOMPOSED_DIGEST_NAMES.getOrDefault(algorithm, algorithm);
     }
 }

src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java

@@ -304,10 +304,6 @@ private boolean checkConstraints(Set<CryptoPrimitive> primitives,
     /**
      * Key and Certificate Constraints
      *
-     * The complete disabling of an algorithm is not handled by Constraints or
-     * Constraint classes.  That is addressed with
-     *   permit(Set<CryptoPrimitive>, String, AlgorithmParameters)
-     *
      * When passing a Key to permit(), the boolean return values follow the
      * same as the interface class AlgorithmConstraints.permit().  This is to
      * maintain compatibility:
@@ -318,7 +314,6 @@ private boolean checkConstraints(Set<CryptoPrimitive> primitives,
      * will be thrown on a failure to better identify why the operation was
      * disallowed.
      */
-
     private static class Constraints {
         private Map<String, List<Constraint>> constraintsMap = new HashMap<>();
 
@@ -341,9 +336,9 @@ public Constraints(String propertyName, Set<String> constraintSet) {
                 // Check if constraint is a complete disabling of an
                 // algorithm or has conditions.
                 int space = constraintEntry.indexOf(' ');
-                String algorithm = AlgorithmDecomposer.hashName(
-                        ((space > 0 ? constraintEntry.substring(0, space) :
-                                constraintEntry)));
+                String algorithm = AlgorithmDecomposer.decomposeDigestName(
+                        space > 0 ? constraintEntry.substring(0, space) :
+                                constraintEntry);
                 List<Constraint> constraintList =
                         constraintsMap.getOrDefault(
                                 algorithm.toUpperCase(Locale.ENGLISH),
@@ -497,7 +492,7 @@ public void permits(String algorithm, ConstraintsParameters cp)
             // Get all signature algorithms to check for constraints
             Set<String> algorithms = new HashSet<>();
             if (algorithm != null) {
-                algorithms.addAll(AlgorithmDecomposer.decomposeOneHash(algorithm));
+                algorithms.addAll(AlgorithmDecomposer.decomposeName(algorithm));
                 algorithms.add(algorithm);
             }
 

src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java

@@ -159,9 +159,9 @@ protected void runClientApplication(SSLSocket socket) throws Exception {
             if (expectFail) {
             // focus on the CertPathValidatorException
                 Throwable t = e.getCause().getCause();
-                if (t == null || !t.toString().contains("MD5withRSA")) {
+                if (t == null || !t.toString().contains("MD5")) {
                     throw new RuntimeException(
-                        "Expected to see MD5withRSA in exception output", t);
+                        "Expected to see MD5 in exception output", t);
                 }
             } else {
                 throw e;

test/jdk/sun/security/ssl/SSLContextImpl/TrustTrustedCert.java

@@ -129,7 +129,7 @@ public static void main(String[] args) throws Exception {
                 " a.jar ee")
                 .shouldContain("Signature algorithm: MD5withRSA (disabled), 2048-bit key")
                 .shouldContain("Signature algorithm: SHA256withRSA, 2048-bit key")
-                .shouldContain("Invalid certificate chain: Algorithm constraints check failed on signature algorithm: MD5withRSA")
+                .shouldContain("Invalid certificate chain: Algorithm constraints check failed on disabled algorithm: MD5 used with certificate: CN=EE")
                 .shouldHaveExitValue(0);
 
         kt("-exportcert -alias cacert -rfc -file cacert", "ks");
@@ -139,7 +139,7 @@ public static void main(String[] args) throws Exception {
                 "-keystore caks1 -storepass changeit -verbose -debug")
                 .shouldContain("Signature algorithm: MD5withRSA (disabled), 2048-bit key")
                 .shouldContain("Signature algorithm: SHA256withRSA, 2048-bit key")
-                .shouldContain("Invalid certificate chain: Algorithm constraints check failed on signature algorithm: MD5withRSA")
+                .shouldContain("Invalid certificate chain: Algorithm constraints check failed on disabled algorithm: MD5 used with certificate: CN=EE")
                 .shouldHaveExitValue(0);
     }
 }

test/jdk/sun/security/tools/jarsigner/CheckSignerCertChain.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2021, 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
@@ -32,7 +32,7 @@
 
 /**
  * @test
- * @bug 8024302 8026037 8130132
+ * @bug 8024302 8026037 8130132 8243585
  * @summary warnings, errors and -strict
  * @library /lib/testlibrary /test/lib
  * @build jdk.test.lib.util.JarUtils
@@ -83,7 +83,7 @@ public static void main(String[] args) throws Throwable {
 
         issueCert("b", "-sigalg MD5withRSA");
         run("jarsigner", "a.jar b")
-                .shouldMatch("chain is invalid. Reason:.*MD5withRSA");
+                .shouldMatch("chain is invalid. Reason:.*MD5.*");
 
         recreateJar();
 
@@ -175,6 +175,18 @@ public static void main(String[] args) throws Throwable {
                 .shouldHaveExitValue(4)
                 .shouldContain("with signer errors")
                 .shouldMatch("(?s).*Error:.*has expired.*Warning:.*");
+
+        // Sign jar with Trust Anchor that has a 512 bit key. Make sure
+        // the error message indicates the key size is restricted.
+        recreateJar();
+        run("keytool", "-delete -alias ca");
+        newCert("ca", "-keysize 512", "-validity 365000", "-ext bc:c");
+        newCert("d");
+        issueCert("d");
+        run("jarsigner", "a.jar d")
+                .shouldContain("chain is invalid. " +
+                        "Reason: Algorithm constraints check failed on " +
+                        "keysize limits: RSA 512 bit key.");
     }
 
     // Creates a new jar without signature