8215776: Keytool importkeystore may mix up certificate chain entries when DNs conflict

Reviewed-by: xuelei

src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1999, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1999, 2019, 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
@@ -63,6 +63,7 @@ import javax.crypto.Mac;
 import javax.security.auth.DestroyFailedException;
 import javax.security.auth.x500.X500Principal;
 
+import sun.security.tools.KeyStoreUtil;
 import sun.security.util.Debug;
 import sun.security.util.DerInputStream;
 import sun.security.util.DerOutputStream;
@@ -74,6 +75,7 @@ import sun.security.x509.AlgorithmId;
 import sun.security.pkcs.EncryptedPrivateKeyInfo;
 import sun.security.provider.JavaKeyStore.JKS;
 import sun.security.util.KeyStoreDelegator;
+import sun.security.x509.AuthorityKeyIdentifierExtension;
 
 
 /**
@@ -306,8 +308,7 @@ public final class PKCS12KeyStore extends KeyStoreSpi {
         Collections.synchronizedMap(new LinkedHashMap<String, Entry>());
 
     private ArrayList<KeyEntry> keyList = new ArrayList<KeyEntry>();
-    private LinkedHashMap<X500Principal, X509Certificate> certsMap =
+    private List<X509Certificate> allCerts = new ArrayList<>();
-            new LinkedHashMap<X500Principal, X509Certificate>();
     private ArrayList<CertEntry> certEntries = new ArrayList<CertEntry>();
 
     /**
@@ -2212,11 +2213,10 @@ public final class PKCS12KeyStore extends KeyStoreSpi {
                         }
                     }
                     chain.add(cert);
-                    X500Principal issuerDN = cert.getIssuerX500Principal();
+                    if (KeyStoreUtil.isSelfSigned(cert)) {
-                    if (issuerDN.equals(cert.getSubjectX500Principal())) {
                         break;
                     }
-                    cert = certsMap.get(issuerDN);
+                    cert = findIssuer(cert);
                 }
                 /* Update existing KeyEntry in entries table */
                 if (chain.size() > 0) {
@@ -2246,10 +2246,74 @@ public final class PKCS12KeyStore extends KeyStoreSpi {
         }
 
         certEntries.clear();
-        certsMap.clear();
+        allCerts.clear();
         keyList.clear();
     }
 
+    /**
+     * Find the issuer of input in allCerts. If the input has an
+     * AuthorityKeyIdentifier extension and the keyId inside matches
+     * the keyId of the SubjectKeyIdentifier of a cert. This cert is
+     * returned. Otherwise, a cert whose subjectDN is the same as the
+     * input's issuerDN is returned.
+     *
+     * @param input the input certificate
+     * @return the isssuer, or null if none matches
+     */
+    private X509Certificate findIssuer(X509Certificate input) {
+
+        X509Certificate fallback = null; // the DN match
+        X500Principal issuerPrinc = input.getIssuerX500Principal();
+
+        // AuthorityKeyIdentifier value encoded as an OCTET STRING
+        byte[] issuerIdExtension = input.getExtensionValue("2.5.29.35");
+        byte[] issuerId = null;
+
+        if (issuerIdExtension != null) {
+            try {
+                issuerId = new AuthorityKeyIdentifierExtension(
+                            false,
+                            new DerValue(issuerIdExtension).getOctetString())
+                        .getEncodedKeyIdentifier();
+            } catch (IOException e) {
+                // ignored. issuerId is still null
+            }
+        }
+
+        for (X509Certificate cert : allCerts) {
+            if (cert.getSubjectX500Principal().equals(issuerPrinc)) {
+                if (issuerId != null) {
+                    // SubjectKeyIdentifier value encoded as an OCTET STRING
+                    byte[] subjectIdExtension = cert.getExtensionValue("2.5.29.14");
+                    byte[] subjectId = null;
+                    if (subjectIdExtension != null) {
+                        try {
+                            subjectId = new DerValue(subjectIdExtension)
+                                    .getOctetString();
+                        } catch (IOException e) {
+                            // ignored. issuerId is still null
+                        }
+                    }
+                    if (subjectId != null) {
+                        if (Arrays.equals(issuerId, subjectId)) {
+                            // keyId exact match!
+                            return cert;
+                        } else {
+                            // Different keyId. Not a fallback.
+                            continue;
+                        }
+                    } else {
+                        // A DN match with no subjectId
+                        fallback = cert;
+                    }
+                } else { // if there is no issuerId, return the 1st DN match
+                    return cert;
+                }
+            }
+        }
+        return fallback;
+    }
+
     /**
      * Returns if a pkcs12 file is password-less. This means no cert is
      * encrypted and there is no Mac. Please note that the private key
@@ -2507,12 +2571,7 @@ public final class PKCS12KeyStore extends KeyStoreSpi {
                 } else {
                     certEntries.add(new CertEntry(cert, keyId, alias));
                 }
-                X500Principal subjectDN = cert.getSubjectX500Principal();
+                allCerts.add(cert);
-                if (subjectDN != null) {
-                    if (!certsMap.containsKey(subjectDN)) {
-                        certsMap.put(subjectDN, cert);
-                    }
-                }
             }
         }
     }

test/jdk/sun/security/pkcs12/SameDN.java

@@ -0,0 +1,70 @@
+/*
+ * Copyright (c) 2019, 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.
+ */
+
+import static jdk.test.lib.SecurityTools.keytool;
+
+import java.io.File;
+import java.security.KeyStore;
+
+/*
+ * @test
+ * @bug 8215776
+ * @library /test/lib
+ * @summary Keytool importkeystore may mix up certificate chain entries when DNs conflict
+ */
+public class SameDN {
+
+    private static final String COMMON = "-keystore ks -storepass changeit ";
+
+    public static final void main(String[] args) throws Exception {
+        genkeypair("ca1", "CN=CA");
+        genkeypair("ca2", "CN=CA");
+        genkeypair("user1", "CN=user");
+        genkeypair("user2", "CN=user");
+        gencert("ca1", "user1");
+        gencert("ca2", "user2");
+
+        KeyStore ks = KeyStore.getInstance(
+                new File("ks"), "changeit".toCharArray());
+        if (!ks.getCertificate("ca1").equals(ks.getCertificateChain("user1")[1])) {
+            throw new Exception("user1 not signed by ca1");
+        }
+        if (!ks.getCertificate("ca2").equals(ks.getCertificateChain("user2")[1])) {
+            throw new Exception("user2 not signed by ca2");
+        }
+    }
+
+    static void genkeypair(String alias, String dn) throws Exception {
+        keytool(COMMON + "-genkeypair -alias " + alias + " -dname " + dn)
+                .shouldHaveExitValue(0);
+    }
+
+    static void gencert(String issuer, String subject) throws Exception {
+        keytool(COMMON + "-certreq -alias " + subject + " -file req")
+                .shouldHaveExitValue(0);
+        keytool(COMMON + "-gencert -alias " + issuer + " -infile req -outfile cert")
+                .shouldHaveExitValue(0);
+        keytool(COMMON + "-importcert -alias " + subject + " -file cert")
+                .shouldHaveExitValue(0);
+    }
+}