8278851: Correct signer logic for jars signed with multiple digest algorithms

Reviewed-by: bae
Backport-of: 61b8944327e3d12cf58dc3f6bc45ecbeba4ef611

Author: Yuri Nesterenko

src/java.base/share/classes/java/util/jar/JarVerifier.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2022, 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
@@ -96,6 +96,10 @@ class JarVerifier {
     /** collect -DIGEST-MANIFEST values for blacklist */
     private List<Object> manifestDigests;
 
+    /* A cache mapping code signers to the algorithms used to digest jar
+       entries, and whether or not the algorithms are permitted. */
+    private Map<CodeSigner[], Map<String, Boolean>> signersToAlgs;
+
     public JarVerifier(String name, byte rawBytes[]) {
         manifestName = name;
         manifestRawBytes = rawBytes;
@@ -105,6 +109,7 @@ public JarVerifier(String name, byte rawBytes[]) {
         pendingBlocks = new ArrayList<>();
         baos = new ByteArrayOutputStream();
         manifestDigests = new ArrayList<>();
+        signersToAlgs = new HashMap<>();
     }
 
     /**
@@ -244,7 +249,8 @@ private void processEntry(ManifestEntryVerifier mev)
         if (!parsingBlockOrSF) {
             JarEntry je = mev.getEntry();
             if ((je != null) && (je.signers == null)) {
-                je.signers = mev.verify(verifiedSigners, sigFileSigners);
+                je.signers = mev.verify(verifiedSigners, sigFileSigners,
+                                        signersToAlgs);
                 je.certs = mapSignersToCertArray(je.signers);
             }
         } else {

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2022, 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
@@ -192,7 +192,8 @@ public JarEntry getEntry()
      *
      */
     public CodeSigner[] verify(Hashtable<String, CodeSigner[]> verifiedSigners,
-                Hashtable<String, CodeSigner[]> sigFileSigners)
+                Hashtable<String, CodeSigner[]> sigFileSigners,
+                Map<CodeSigner[], Map<String, Boolean>> signersToAlgs)
         throws JarException
     {
         if (skip) {
@@ -207,38 +208,60 @@ public CodeSigner[] verify(Hashtable<String, CodeSigner[]> verifiedSigners,
             return signers;
         }
 
-        JarConstraintsParameters params =
-            getParams(verifiedSigners, sigFileSigners);
+        CodeSigner[] entrySigners = sigFileSigners.get(name);
+        Map<String, Boolean> algsPermittedStatus =
+            algsPermittedStatusForSigners(signersToAlgs, entrySigners);
 
+        // Flag that indicates if only disabled algorithms are used and jar
+        // entry should be treated as unsigned.
+        boolean disabledAlgs = true;
+        JarConstraintsParameters params = null;
         for (int i=0; i < digests.size(); i++) {
             MessageDigest digest = digests.get(i);
-            if (params != null) {
-                try {
-                    params.setExtendedExceptionMsg(JarFile.MANIFEST_NAME,
-                        name + " entry");
-                    DisabledAlgorithmConstraints.jarConstraints()
-                           .permits(digest.getAlgorithm(), params);
-                } catch (GeneralSecurityException e) {
-                    if (debug != null) {
-                        debug.println("Digest algorithm is restricted: " + e);
+            String digestAlg = digest.getAlgorithm();
+
+            // Check if this algorithm is permitted, skip if false.
+            if (algsPermittedStatus != null) {
+                Boolean permitted = algsPermittedStatus.get(digestAlg);
+                if (permitted == null) {
+                    if (params == null) {
+                        params = new JarConstraintsParameters(entrySigners);
                     }
-                    return null;
+                    if (!checkConstraints(digestAlg, params)) {
+                        algsPermittedStatus.put(digestAlg, Boolean.FALSE);
+                        continue;
+                    } else {
+                        algsPermittedStatus.put(digestAlg, Boolean.TRUE);
+                    }
+                } else if (!permitted) {
+                    continue;
                 }
             }
+
+            // A non-disabled algorithm was used.
+            disabledAlgs = false;
+
             byte [] manHash = manifestHashes.get(i);
             byte [] theHash = digest.digest();
 
             if (debug != null) {
                 debug.println("Manifest Entry: " +
-                                   name + " digest=" + digest.getAlgorithm());
+                                   name + " digest=" + digestAlg);
                 debug.println("  manifest " + toHex(manHash));
                 debug.println("  computed " + toHex(theHash));
                 debug.println();
             }
 
-            if (!MessageDigest.isEqual(theHash, manHash))
-                throw new SecurityException(digest.getAlgorithm()+
+            if (!MessageDigest.isEqual(theHash, manHash)) {
+                throw new SecurityException(digestAlg +
                                             " digest error for "+name);
+            }
+        }
+
+        // If there were only disabled algorithms used, return null and jar
+        // entry will be treated as unsigned.
+        if (disabledAlgs) {
+            return null;
         }
 
         // take it out of sigFileSigners and put it in verifiedSigners...
@@ -249,39 +272,36 @@ public CodeSigner[] verify(Hashtable<String, CodeSigner[]> verifiedSigners,
         return signers;
     }
 
-    /**
-     * Get constraints parameters for JAR. The constraints should be
-     * checked against all code signers. Returns the parameters,
-     * or null if the signers for this entry have already been checked
-     * or there are no signers for this entry.
-     */
-    private JarConstraintsParameters getParams(
-            Map<String, CodeSigner[]> verifiedSigners,
-            Map<String, CodeSigner[]> sigFileSigners) {
-
-        // verifiedSigners is usually preloaded with the Manifest's signers.
-        // If verifiedSigners contains the Manifest, then it will have all of
-        // the signers of the JAR. But if it doesn't then we need to fallback
-        // and check verifiedSigners to see if the signers of this entry have
-        // been checked already.
-        if (verifiedSigners.containsKey(manifestFileName)) {
-            if (verifiedSigners.size() > 1) {
-                // this means we already checked it previously
-                return null;
-            } else {
-                return new JarConstraintsParameters(
-                    verifiedSigners.get(manifestFileName));
+    // Gets the algorithms permitted status for the signers of this entry.
+    private static Map<String, Boolean> algsPermittedStatusForSigners(
+            Map<CodeSigner[], Map<String, Boolean>> signersToAlgs,
+            CodeSigner[] signers) {
+        if (signers != null) {
+            Map<String, Boolean> algs = signersToAlgs.get(signers);
+            // create new HashMap if absent
+            if (algs == null) {
+                algs = new HashMap<>();
+                signersToAlgs.put(signers, algs);
             }
-        } else {
+            return algs;
+        }
+        return null;
+    }
+
+    // Checks the algorithm constraints against the signers of this entry.
+    private boolean checkConstraints(String algorithm,
+        JarConstraintsParameters params) {
+        try {
+            params.setExtendedExceptionMsg(JarFile.MANIFEST_NAME,
+                name + " entry");
+            DisabledAlgorithmConstraints.jarConstraints()
+                   .permits(algorithm, params);
+            return true;
+        } catch (GeneralSecurityException e) {
             if (debug != null) {
-                debug.println(manifestFileName + " not present in verifiedSigners");
-            }
-            CodeSigner[] signers = sigFileSigners.get(name);
-            if (signers == null || verifiedSigners.containsValue(signers)) {
-                return null;
-            } else {
-                return new JarConstraintsParameters(signers);
+                debug.println("Digest algorithm is restricted: " + e);
             }
+            return false;
         }
     }
 

test/jdk/jdk/security/jarsigner/JarWithOneNonDisabledDigestAlg.java

@@ -0,0 +1,169 @@
+/*
+ * Copyright (c) 2022, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8278851
+ * @summary Check that jar entry with at least one non-disabled digest
+ *          algorithm in manifest is treated as signed
+ * @modules java.base/sun.security.tools.keytool
+ * @library /test/lib
+ * @build jdk.test.lib.util.JarUtils
+ *        jdk.test.lib.security.SecurityUtils
+ * @run main/othervm JarWithOneNonDisabledDigestAlg
+ */
+
+import java.io.InputStream;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.security.CodeSigner;
+import java.security.KeyStore;
+import java.util.Enumeration;
+import java.util.List;
+import java.util.Locale;
+import java.util.jar.JarEntry;
+import java.util.jar.JarFile;
+import java.util.zip.ZipFile;
+import jdk.security.jarsigner.JarSigner;
+
+import jdk.test.lib.util.JarUtils;
+import jdk.test.lib.security.SecurityUtils;
+
+public class JarWithOneNonDisabledDigestAlg {
+
+    private static final String PASS = "changeit";
+    private static final String TESTFILE1 = "testfile1";
+    private static final String TESTFILE2 = "testfile2";
+
+    public static void main(String[] args) throws Exception {
+        SecurityUtils.removeFromDisabledAlgs("jdk.jar.disabledAlgorithms",
+            List.of("SHA1"));
+        Files.write(Path.of(TESTFILE1), TESTFILE1.getBytes());
+        JarUtils.createJarFile(Path.of("unsigned.jar"), Path.of("."),
+            Path.of(TESTFILE1));
+
+        genkeypair("-alias SHA1 -sigalg SHA1withRSA");
+        genkeypair("-alias SHA256 -sigalg SHA256withRSA");
+
+        KeyStore ks = KeyStore.getInstance(KeyStore.getDefaultType());
+        try (FileInputStream fis = new FileInputStream("keystore")) {
+            ks.load(fis, PASS.toCharArray());
+        }
+
+        // Sign JAR twice with same signer but different digest algorithms
+        // so that each entry in manifest file contains two digest values.
+        signJarFile(ks, "SHA1", "MD5", "unsigned.jar", "signed.jar");
+        signJarFile(ks, "SHA1", "SHA1", "signed.jar", "signed2.jar");
+        checkThatJarIsSigned("signed2.jar", false);
+
+        // add another file to the JAR
+        Files.write(Path.of(TESTFILE2), "testFile2".getBytes());
+        JarUtils.updateJarFile(Path.of("signed2.jar"), Path.of("."),
+            Path.of(TESTFILE2));
+
+        // Sign again with different signer (SHA256) and SHA-1 digestalg.
+        // TESTFILE1 should have two signers and TESTFILE2 should have one
+        // signer.
+        signJarFile(ks, "SHA256", "SHA1", "signed2.jar", "multi-signed.jar");
+
+        checkThatJarIsSigned("multi-signed.jar", true);
+    }
+
+    private static KeyStore.PrivateKeyEntry getEntry(KeyStore ks, String alias)
+        throws Exception {
+
+        return (KeyStore.PrivateKeyEntry)
+            ks.getEntry(alias,
+                new KeyStore.PasswordProtection(PASS.toCharArray()));
+    }
+
+    private static void genkeypair(String cmd) throws Exception {
+        cmd = "-genkeypair -keystore keystore -storepass " + PASS +
+              " -keypass " + PASS + " -keyalg rsa -dname CN=Duke " + cmd;
+        sun.security.tools.keytool.Main.main(cmd.split(" "));
+    }
+
+    private static void signJarFile(KeyStore ks, String alias,
+        String digestAlg, String inputFile, String outputFile)
+        throws Exception {
+
+        JarSigner signer = new JarSigner.Builder(getEntry(ks, alias))
+                 .digestAlgorithm(digestAlg)
+                 .signerName(alias)
+                 .build();
+
+        try (ZipFile in = new ZipFile(inputFile);
+            FileOutputStream out = new FileOutputStream(outputFile)) {
+            signer.sign(in, out);
+        }
+    }
+
+    private static void checkThatJarIsSigned(String jarFile, boolean multi)
+        throws Exception {
+
+        try (JarFile jf = new JarFile(jarFile, true)) {
+            Enumeration<JarEntry> entries = jf.entries();
+            while (entries.hasMoreElements()) {
+                JarEntry entry = entries.nextElement();
+                if (entry.isDirectory() || isSigningRelated(entry.getName())) {
+                    continue;
+                }
+                InputStream is = jf.getInputStream(entry);
+                while (is.read() != -1);
+                CodeSigner[] signers = entry.getCodeSigners();
+                if (signers == null) {
+                    throw new Exception("JarEntry " + entry.getName() +
+                        " is not signed");
+                } else if (multi) {
+                    if (entry.getName().equals(TESTFILE1) &&
+                        signers.length != 2) {
+                        throw new Exception("Unexpected number of signers " +
+                            "for " + entry.getName() + ": " + signers.length);
+                    } else if (entry.getName().equals(TESTFILE2) &&
+                        signers.length != 1) {
+                        throw new Exception("Unexpected number of signers " +
+                            "for " + entry.getName() + ": " + signers.length);
+                    }
+                }
+            }
+        }
+    }
+
+    private static boolean isSigningRelated(String name) {
+        name = name.toUpperCase(Locale.ENGLISH);
+        if (!name.startsWith("META-INF/")) {
+            return false;
+        }
+        name = name.substring(9);
+        if (name.indexOf('/') != -1) {
+            return false;
+        }
+        return name.endsWith(".SF")
+            || name.endsWith(".DSA")
+            || name.endsWith(".RSA")
+            || name.endsWith(".EC")
+            || name.equals("MANIFEST.MF");
+    }
+}

test/lib/jdk/test/lib/security/SecurityUtils.java

@@ -61,11 +61,18 @@ public static void removeFromDisabledTlsAlgs(String... protocols) {
                                List.<String>of(protocols));
     }
 
-    private static void removeFromDisabledAlgs(String prop, List<String> algs) {
+    /**
+     * Removes constraints that contain the specified constraint from the
+     * specified security property. For example, List.of("SHA1") will remove
+     * any constraint containing "SHA1".
+     */
+    public static void removeFromDisabledAlgs(String prop,
+            List<String> constraints) {
         String value = Security.getProperty(prop);
         value = Arrays.stream(value.split(","))
                       .map(s -> s.trim())
-                      .filter(s -> !algs.contains(s))
+                      .filter(s -> constraints.stream()
+                          .allMatch(constraint -> !s.contains(constraint)))
                       .collect(Collectors.joining(","));
         Security.setProperty(prop, value);
     }