8215776: Keytool importkeystore may mix up certificate chain entries when DNs conflict
Reviewed-by: xuelei
--- a/src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java Tue Jan 22 12:32:19 2019 +0000
+++ b/src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java Tue Jan 22 21:18:45 2019 +0800
@@ -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.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.pkcs.EncryptedPrivateKeyInfo;
import sun.security.provider.JavaKeyStore.JKS;
import sun.security.util.KeyStoreDelegator;
+import sun.security.x509.AuthorityKeyIdentifierExtension;
/**
@@ -306,8 +308,7 @@
Collections.synchronizedMap(new LinkedHashMap<String, Entry>());
private ArrayList<KeyEntry> keyList = new ArrayList<KeyEntry>();
- private LinkedHashMap<X500Principal, X509Certificate> certsMap =
- new LinkedHashMap<X500Principal, X509Certificate>();
+ private List<X509Certificate> allCerts = new ArrayList<>();
private ArrayList<CertEntry> certEntries = new ArrayList<CertEntry>();
/**
@@ -2212,11 +2213,10 @@
}
}
chain.add(cert);
- X500Principal issuerDN = cert.getIssuerX500Principal();
- if (issuerDN.equals(cert.getSubjectX500Principal())) {
+ if (KeyStoreUtil.isSelfSigned(cert)) {
break;
}
- cert = certsMap.get(issuerDN);
+ cert = findIssuer(cert);
}
/* Update existing KeyEntry in entries table */
if (chain.size() > 0) {
@@ -2246,11 +2246,75 @@
}
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
* can be encrypted.
@@ -2507,12 +2571,7 @@
} else {
certEntries.add(new CertEntry(cert, keyId, alias));
}
- X500Principal subjectDN = cert.getSubjectX500Principal();
- if (subjectDN != null) {
- if (!certsMap.containsKey(subjectDN)) {
- certsMap.put(subjectDN, cert);
- }
- }
+ allCerts.add(cert);
}
}
}
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/sun/security/pkcs12/SameDN.java Tue Jan 22 21:18:45 2019 +0800
@@ -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);
+ }
+}