Skip to content

Commit 6b27dad

Browse files
author
Valerie Peng
committedMay 23, 2023
8301154: SunPKCS11 KeyStore deleteEntry results in dangling PrivateKey entries
Reviewed-by: weijun, hchao
1 parent ed0e956 commit 6b27dad

File tree

4 files changed

+320
-84
lines changed

4 files changed

+320
-84
lines changed
 

‎src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyStore.java

+80-84
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2003, 2022, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2003, 2023, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -784,7 +784,7 @@ public synchronized void engineLoad(InputStream stream, char[] password)
784784
writeDisabled = true;
785785
}
786786
if (debug != null) {
787-
dumpTokenMap();
787+
dumpTokenMap(debug);
788788
debug.println("P11KeyStore load. Entry count: " +
789789
aliasMap.size());
790790
}
@@ -866,7 +866,7 @@ public synchronized void engineLoad(KeyStore.LoadStoreParameter param)
866866
writeDisabled = true;
867867
}
868868
if (debug != null) {
869-
dumpTokenMap();
869+
dumpTokenMap(debug);
870870
}
871871
} catch (LoginException | KeyStoreException | PKCS11Exception e) {
872872
throw new IOException("load failed", e);
@@ -1152,7 +1152,7 @@ public synchronized void engineSetEntry(String alias, KeyStore.Entry entry,
11521152

11531153
mapLabels();
11541154
if (debug != null) {
1155-
dumpTokenMap();
1155+
dumpTokenMap(debug);
11561156
}
11571157
} catch (PKCS11Exception | CertificateException pe) {
11581158
throw new KeyStoreException(pe);
@@ -1229,7 +1229,7 @@ private X509Certificate[] loadChain(Session session,
12291229
next.getIssuerX500Principal().getEncoded()) };
12301230
long[] ch = findObjects(session, attrs);
12311231

1232-
if (ch == null || ch.length == 0) {
1232+
if (ch.length == 0) {
12331233
// done
12341234
break;
12351235
} else {
@@ -1980,91 +1980,86 @@ private boolean destroyChain(byte[] cka_id)
19801980
return false;
19811981
}
19821982

1983-
X509Certificate endCert = loadCert(session, h.handle);
1984-
token.p11.C_DestroyObject(session.id(), h.handle);
1985-
if (debug != null) {
1986-
debug.println("destroyChain destroyed end entity cert " +
1987-
"with CKA_ID [" +
1988-
getIDNullSafe(cka_id) +
1989-
"]");
1990-
}
1991-
1992-
// build chain following issuer->subject links
1993-
1994-
X509Certificate next = endCert;
1995-
while (true) {
1996-
1997-
if (next.getSubjectX500Principal().equals
1998-
(next.getIssuerX500Principal())) {
1999-
// self-signed - done
2000-
break;
2001-
}
1983+
long currHdl = h.handle;
1984+
boolean checkPrivKey = false;
1985+
while (currHdl != 0L) {
1986+
X509Certificate cert = loadCert(session, currHdl);
1987+
boolean selfSigned = cert.getSubjectX500Principal().equals
1988+
(cert.getIssuerX500Principal());
20021989

1990+
// only delete if both of the followings are true
1991+
// 1) no other certs depend on it
1992+
// 2) not corresponds to any private key
20031993
CK_ATTRIBUTE[] attrs = new CK_ATTRIBUTE[] {
2004-
ATTR_TOKEN_TRUE,
2005-
ATTR_CLASS_CERT,
2006-
new CK_ATTRIBUTE(CKA_SUBJECT,
2007-
next.getIssuerX500Principal().getEncoded()) };
2008-
long[] ch = findObjects(session, attrs);
2009-
2010-
if (ch == null || ch.length == 0) {
2011-
// done
2012-
break;
2013-
} else {
2014-
// if more than one found, use first
2015-
if (debug != null && ch.length > 1) {
2016-
debug.println("destroyChain found " +
2017-
ch.length +
2018-
" certificate entries for subject [" +
2019-
next.getIssuerX500Principal() +
2020-
"] in token - using first entry");
2021-
}
2022-
2023-
next = loadCert(session, ch[0]);
1994+
ATTR_TOKEN_TRUE,
1995+
ATTR_CLASS_CERT,
1996+
new CK_ATTRIBUTE(CKA_ISSUER,
1997+
cert.getSubjectX500Principal().getEncoded())
1998+
};
1999+
boolean destroyIt = true;
20242000

2025-
// only delete if not part of any other chain
2001+
long[] dependents = findObjects(session, attrs);
2002+
if (dependents.length > 1 ||
2003+
(!selfSigned && dependents.length == 1)) {
2004+
destroyIt = false;
2005+
}
20262006

2007+
if (destroyIt && checkPrivKey) {
2008+
// proceed with checking if there is a private key
20272009
attrs = new CK_ATTRIBUTE[] {
2028-
ATTR_TOKEN_TRUE,
2029-
ATTR_CLASS_CERT,
2030-
new CK_ATTRIBUTE(CKA_ISSUER,
2031-
next.getSubjectX500Principal().getEncoded()) };
2032-
long[] issuers = findObjects(session, attrs);
2033-
2034-
boolean destroyIt = false;
2035-
if (issuers == null || issuers.length == 0) {
2036-
// no other certs with this issuer -
2037-
// destroy it
2038-
destroyIt = true;
2039-
} else if (issuers.length == 1) {
2040-
X509Certificate iCert = loadCert(session, issuers[0]);
2041-
if (next.equals(iCert)) {
2042-
// only cert with issuer is itself (self-signed) -
2043-
// destroy it
2044-
destroyIt = true;
2045-
}
2010+
new CK_ATTRIBUTE(CKA_ID),
2011+
};
2012+
token.p11.C_GetAttributeValue(session.id(), currHdl, attrs);
2013+
byte[] currId = attrs[0].getByteArray();
2014+
if (currId != null) {
2015+
attrs = new CK_ATTRIBUTE[] {
2016+
ATTR_TOKEN_TRUE,
2017+
ATTR_CLASS_PKEY,
2018+
new CK_ATTRIBUTE(CKA_ID, currId)
2019+
};
2020+
long[] privKeys = findObjects(session, attrs);
2021+
destroyIt = privKeys.length == 0;
20462022
}
2047-
2048-
if (destroyIt) {
2049-
token.p11.C_DestroyObject(session.id(), ch[0]);
2050-
if (debug != null) {
2051-
debug.println
2052-
("destroyChain destroyed cert in chain " +
2053-
"with subject [" +
2054-
next.getSubjectX500Principal() + "]");
2055-
}
2056-
} else {
2057-
if (debug != null) {
2058-
debug.println("destroyChain did not destroy " +
2059-
"shared cert in chain with subject [" +
2060-
next.getSubjectX500Principal() + "]");
2061-
}
2023+
}
2024+
if (destroyIt) {
2025+
token.p11.C_DestroyObject(session.id(), currHdl);
2026+
if (debug != null) {
2027+
debug.println("destroyChain destroyed cert in chain " +
2028+
"with subject [" +
2029+
cert.getSubjectX500Principal() + "]");
2030+
}
2031+
} else {
2032+
if (debug != null) {
2033+
debug.println("destroyChain did not destroy " +
2034+
"shared cert in chain with subject [" +
2035+
cert.getSubjectX500Principal() + "]");
20622036
}
20632037
}
2038+
if (selfSigned) {
2039+
break; // done
2040+
}
2041+
attrs = new CK_ATTRIBUTE[] {
2042+
ATTR_TOKEN_TRUE,
2043+
ATTR_CLASS_CERT,
2044+
new CK_ATTRIBUTE(CKA_SUBJECT,
2045+
cert.getIssuerX500Principal().getEncoded())
2046+
};
2047+
long[] ch = findObjects(session, attrs);
2048+
if (ch.length == 0) {
2049+
break;
2050+
}
2051+
// if more than one found, use first
2052+
if (debug != null && ch.length > 1) {
2053+
debug.println("destroyChain found " +
2054+
ch.length +
2055+
" certificate entries for subject [" +
2056+
cert.getIssuerX500Principal() +
2057+
"] in token - using first entry");
2058+
}
2059+
currHdl = ch[0];
2060+
checkPrivKey = true;
20642061
}
2065-
20662062
return true;
2067-
20682063
} finally {
20692064
token.releaseSession(session);
20702065
}
@@ -2646,14 +2641,14 @@ private void mapSecretKeys(HashMap<String, AliasInfo> sKeyMap)
26462641
aliasMap.putAll(sKeyMap);
26472642
}
26482643

2649-
private void dumpTokenMap() {
2644+
private void dumpTokenMap(Debug debug) {
26502645
Set<String> aliases = aliasMap.keySet();
2651-
System.out.println("Token Alias Map:");
2646+
debug.println("Token Alias Map:");
26522647
if (aliases.isEmpty()) {
2653-
System.out.println(" [empty]");
2648+
debug.println(" [empty]");
26542649
} else {
26552650
for (String s : aliases) {
2656-
System.out.println(" " + s + aliasMap.get(s));
2651+
debug.println(" " + s + aliasMap.get(s));
26572652
}
26582653
}
26592654
}
@@ -2667,6 +2662,7 @@ private void checkWrite() throws KeyStoreException {
26672662

26682663
private static final long[] LONG0 = new long[0];
26692664

2665+
// return an empty array if no match
26702666
private static long[] findObjects(Session session, CK_ATTRIBUTE[] attrs)
26712667
throws PKCS11Exception {
26722668
Token token = session.token;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,231 @@
1+
/*
2+
* Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
3+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
4+
*
5+
* This code is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU General Public License version 2 only, as
7+
* published by the Free Software Foundation.
8+
*
9+
* This code is distributed in the hope that it will be useful, but WITHOUT
10+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
11+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
12+
* version 2 for more details (a copy is included in the LICENSE file that
13+
* accompanied this code).
14+
*
15+
* You should have received a copy of the GNU General Public License version
16+
* 2 along with this work; if not, write to the Free Software Foundation,
17+
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
18+
*
19+
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
20+
* or visit www.oracle.com if you need additional information or have any
21+
* questions.
22+
*/
23+
24+
/* @test
25+
* @bug 8301154
26+
* @summary test cert chain deletion logic w/ NSS PKCS11 KeyStore
27+
* @library /test/lib ..
28+
* @run testng/othervm CertChainRemoval
29+
*/
30+
import jdk.test.lib.SecurityTools;
31+
import java.io.*;
32+
import java.nio.file.Path;
33+
import java.util.*;
34+
35+
import java.security.Key;
36+
import java.security.KeyStore;
37+
import java.security.KeyStoreException;
38+
import java.security.Provider;
39+
import java.security.cert.Certificate;
40+
41+
import org.testng.annotations.BeforeClass;
42+
import org.testng.annotations.Test;
43+
44+
45+
public class CertChainRemoval extends PKCS11Test {
46+
47+
private static final Path TEST_DATA_PATH = Path.of(BASE)
48+
.resolve("CertChainRemoval");
49+
private static final String DIR = TEST_DATA_PATH.toString();
50+
51+
private record KeyStoreInfo(File file, String type, char[] passwd) {}
52+
53+
private static final KeyStoreInfo TEMP = new KeyStoreInfo(
54+
new File(DIR, "temp.ks"),
55+
"JKS",
56+
new char[] { 'c', 'h', 'a', 'n', 'g', 'e', 'i', 't' });
57+
58+
private static final KeyStoreInfo PKCS11KS = new KeyStoreInfo(
59+
null,
60+
"PKCS11",
61+
new char[] { 't', 'e', 's', 't', '1', '2' });
62+
63+
@BeforeClass
64+
public void setUp() throws Exception {
65+
copyNssCertKeyToClassesDir();
66+
setCommonSystemProps();
67+
// if temp keystore already exists; skip the creation
68+
if (!TEMP.file.exists()) {
69+
createKeyStore(TEMP);
70+
}
71+
System.setProperty("CUSTOM_P11_CONFIG",
72+
TEST_DATA_PATH.resolve("p11-nss.txt").toString());
73+
}
74+
75+
@Test
76+
public void test() throws Exception {
77+
main(new CertChainRemoval());
78+
}
79+
80+
private static void printKeyStore(String header, KeyStore ks)
81+
throws KeyStoreException {
82+
System.out.println(header);
83+
Enumeration enu = ks.aliases();
84+
int count = 0;
85+
while (enu.hasMoreElements()) {
86+
count++;
87+
System.out.println("Entry# " + count +
88+
" = " + (String)enu.nextElement());
89+
}
90+
System.out.println("========");
91+
}
92+
93+
private static void checkEntry(KeyStore ks, String alias,
94+
Certificate[] expChain) throws KeyStoreException {
95+
Certificate c = ks.getCertificate(alias);
96+
Certificate[] chain = ks.getCertificateChain(alias);
97+
if (expChain == null) {
98+
if (c != null || (chain != null && chain.length != 0)) {
99+
throw new RuntimeException("Fail: " + alias + " not removed");
100+
}
101+
} else {
102+
if (!c.equals(expChain[0]) || !Arrays.equals(chain, expChain)) {
103+
System.out.println("expChain: " + expChain.length);
104+
System.out.println("actualChain: " + chain.length);
105+
throw new RuntimeException("Fail: " + alias +
106+
" chain check diff");
107+
}
108+
}
109+
}
110+
111+
public void main(Provider p) throws Exception {
112+
KeyStore sunks = KeyStore.getInstance(TEMP.type, "SUN");
113+
sunks.load(new FileInputStream(TEMP.file), TEMP.passwd);
114+
printKeyStore("Starting with: ", sunks);
115+
116+
KeyStore p11ks;
117+
try {
118+
p11ks = KeyStore.getInstance(PKCS11KS.type, p);
119+
p11ks.load(null, PKCS11KS.passwd);
120+
printKeyStore("Initial PKCS11 KeyStore: ", p11ks);
121+
} catch (Exception e) {
122+
System.out.println("Skip test, due to " + e);
123+
return;
124+
}
125+
126+
// get the necessary keys from the temp keystore
127+
Key pk1PrivKey = sunks.getKey("pk1", TEMP.passwd);
128+
Certificate pk1Cert = sunks.getCertificate("pk1");
129+
Key caPrivKey = sunks.getKey("ca1", TEMP.passwd);
130+
Certificate ca1Cert = sunks.getCertificate("ca1");
131+
Key rootPrivKey = sunks.getKey("root", TEMP.passwd);
132+
Certificate rootCert = sunks.getCertificate("root");
133+
134+
Certificate[] pk1Chain = { pk1Cert, ca1Cert, rootCert };
135+
Certificate[] ca1Chain = { ca1Cert, rootCert };
136+
Certificate[] rootChain = { rootCert };
137+
138+
// populate keystore with "pk1" and "ca", then delete "pk1"
139+
System.out.println("Add pk1, ca1 and root, then delete pk1");
140+
p11ks.setKeyEntry("pk1", pk1PrivKey, null, pk1Chain);
141+
p11ks.setKeyEntry("ca1", caPrivKey, null, ca1Chain);
142+
p11ks.setKeyEntry("root", rootPrivKey, null, rootChain);
143+
p11ks.deleteEntry("pk1");
144+
145+
// reload the keystore
146+
p11ks.store(null, PKCS11KS.passwd);
147+
p11ks.load(null, PKCS11KS.passwd);
148+
printKeyStore("Reload#1: ca1 / root", p11ks);
149+
150+
// should only have "ca1" and "root"
151+
checkEntry(p11ks, "pk1", null);
152+
checkEntry(p11ks, "ca1", ca1Chain);
153+
checkEntry(p11ks, "root", rootChain);
154+
155+
// now add "pk1" and delete "ca1"
156+
System.out.println("Now add pk1 and delete ca1");
157+
p11ks.setKeyEntry("pk1", pk1PrivKey, null, pk1Chain);
158+
p11ks.deleteEntry("ca1");
159+
160+
// reload the keystore
161+
p11ks.store(null, PKCS11KS.passwd);
162+
p11ks.load(null, PKCS11KS.passwd);
163+
printKeyStore("Reload#2: pk1 / root", p11ks);
164+
165+
// should only have "pk1" and "root" now
166+
checkEntry(p11ks, "pk1", pk1Chain);
167+
checkEntry(p11ks, "ca1", null);
168+
checkEntry(p11ks, "root", rootChain);
169+
170+
// now delete "root"
171+
System.out.println("Now delete root");
172+
p11ks.deleteEntry("root");
173+
174+
// reload the keystore
175+
p11ks.store(null, PKCS11KS.passwd);
176+
p11ks.load(null, PKCS11KS.passwd);
177+
printKeyStore("Reload#3: pk1", p11ks);
178+
179+
// should only have "pk1" now
180+
checkEntry(p11ks, "pk1", pk1Chain);
181+
checkEntry(p11ks, "ca1", null);
182+
checkEntry(p11ks, "root", null);
183+
184+
// now delete "pk1"
185+
System.out.println("Now delete pk1");
186+
p11ks.deleteEntry("pk1");
187+
188+
// reload the keystore
189+
p11ks.store(null, PKCS11KS.passwd);
190+
p11ks.load(null, PKCS11KS.passwd);
191+
printKeyStore("Reload#4: ", p11ks);
192+
193+
// should have nothing now
194+
checkEntry(p11ks, "pk1", null);
195+
checkEntry(p11ks, "ca1", null);
196+
checkEntry(p11ks, "root", null);
197+
198+
System.out.println("Test Passed");
199+
}
200+
201+
private static void createKeyStore(KeyStoreInfo ksi) throws Exception {
202+
System.out.println("Creating keypairs and storing them into " +
203+
ksi.file.getAbsolutePath());
204+
String keyGenOptions = " -keyalg RSA -keysize 2048 ";
205+
String keyStoreOptions = " -keystore " + ksi.file.getAbsolutePath() +
206+
" -storetype " + ksi.type + " -storepass " +
207+
new String(ksi.passwd);
208+
209+
String[] aliases = { "ROOT", "CA1", "PK1" };
210+
for (String n : aliases) {
211+
SecurityTools.keytool("-genkeypair -alias " + n +
212+
" -dname CN=" + n + keyGenOptions + keyStoreOptions);
213+
String issuer = switch (n) {
214+
case "CA1"-> "ROOT";
215+
case "PK1"-> "CA1";
216+
default-> null;
217+
};
218+
if (issuer != null) {
219+
// export CSR and issue the cert using the issuer
220+
SecurityTools.keytool("-certreq -alias " + n +
221+
" -file tmp.req" + keyStoreOptions);
222+
SecurityTools.keytool("-gencert -alias " + issuer +
223+
" -infile tmp.req -outfile tmp.cert -validity 3650" +
224+
keyStoreOptions);
225+
SecurityTools.keytool("-importcert -alias " + n +
226+
" -file tmp.cert" + keyStoreOptions);
227+
}
228+
}
229+
}
230+
231+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
name = nss
2+
3+
slot = 2
4+
5+
library = ${pkcs11test.nss.lib}
6+
7+
nssArgs = "configdir='${pkcs11test.nss.db}' certPrefix='' keyPrefix='' secmod='secmod.db'"
8+
9+
attributes = compatibility
Binary file not shown.

1 commit comments

Comments
 (1)

openjdk-notifier[bot] commented on May 23, 2023

@openjdk-notifier[bot]
Please sign in to comment.