8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec

Co-authored-by: Greg Rubin <rubin@amazon.com>
Reviewed-by: valeriep
This commit is contained in:
Ziyi Luo 2021-03-29 20:14:47 +00:00 committed by Valerie Peng
parent 128c0c97a5
commit a5d7de2351
6 changed files with 287 additions and 58 deletions

View File

@ -406,6 +406,7 @@ public class RSAKeyFactory extends KeyFactorySpi {
if (keySpec.isAssignableFrom(PKCS8_KEYSPEC_CLS)) {
return keySpec.cast(new PKCS8EncodedKeySpec(key.getEncoded()));
} else if (keySpec.isAssignableFrom(RSA_PRIVCRT_KEYSPEC_CLS)) {
// All supported keyspecs (other than PKCS8_KEYSPEC_CLS) descend from RSA_PRIVCRT_KEYSPEC_CLS
if (key instanceof RSAPrivateCrtKey) {
RSAPrivateCrtKey crtKey = (RSAPrivateCrtKey)key;
return keySpec.cast(new RSAPrivateCrtKeySpec(
@ -419,17 +420,20 @@ public class RSAKeyFactory extends KeyFactorySpi {
crtKey.getCrtCoefficient(),
crtKey.getParams()
));
} else {
throw new InvalidKeySpecException
("RSAPrivateCrtKeySpec can only be used with CRT keys");
} else { // RSAPrivateKey (non-CRT)
if (!keySpec.isAssignableFrom(RSA_PRIV_KEYSPEC_CLS)) {
throw new InvalidKeySpecException
("RSAPrivateCrtKeySpec can only be used with CRT keys");
}
// fall through to RSAPrivateKey (non-CRT)
RSAPrivateKey rsaKey = (RSAPrivateKey) key;
return keySpec.cast(new RSAPrivateKeySpec(
rsaKey.getModulus(),
rsaKey.getPrivateExponent(),
rsaKey.getParams()
));
}
} else if (keySpec.isAssignableFrom(RSA_PRIV_KEYSPEC_CLS)) {
RSAPrivateKey rsaKey = (RSAPrivateKey)key;
return keySpec.cast(new RSAPrivateKeySpec(
rsaKey.getModulus(),
rsaKey.getPrivateExponent(),
rsaKey.getParams()
));
} else {
throw new InvalidKeySpecException
("KeySpec must be RSAPrivate(Crt)KeySpec or "

View File

@ -277,54 +277,51 @@ final class P11RSAKeyFactory extends P11KeyFactory {
<T extends KeySpec> T implGetPrivateKeySpec(P11Key key, Class<T> keySpec,
Session[] session) throws PKCS11Exception, InvalidKeySpecException {
if (key.sensitive || !key.extractable) {
throw new InvalidKeySpecException("Key is sensitive or not extractable");
}
// If the key is both extractable and not sensitive, then when it was converted into a P11Key
// it was also converted into subclass of RSAPrivateKey which encapsulates all of the logic
// necessary to retrieve the attributes we need. This sub-class will also cache these attributes
// so that we do not need to query them more than once.
// Rather than rewrite this logic and make possibly slow calls to the token, we'll just use
// that existing logic.
if (keySpec.isAssignableFrom(RSAPrivateCrtKeySpec.class)) {
session[0] = token.getObjSession();
CK_ATTRIBUTE[] attributes = new CK_ATTRIBUTE[] {
new CK_ATTRIBUTE(CKA_MODULUS),
new CK_ATTRIBUTE(CKA_PUBLIC_EXPONENT),
new CK_ATTRIBUTE(CKA_PRIVATE_EXPONENT),
new CK_ATTRIBUTE(CKA_PRIME_1),
new CK_ATTRIBUTE(CKA_PRIME_2),
new CK_ATTRIBUTE(CKA_EXPONENT_1),
new CK_ATTRIBUTE(CKA_EXPONENT_2),
new CK_ATTRIBUTE(CKA_COEFFICIENT),
};
long keyID = key.getKeyID();
try {
token.p11.C_GetAttributeValue(session[0].id(), keyID, attributes);
} finally {
key.releaseKeyID();
}
// All supported keyspecs (other than PKCS8EncodedKeySpec) descend from RSAPrivateCrtKeySpec
if (key instanceof RSAPrivateCrtKey) {
RSAPrivateCrtKey crtKey = (RSAPrivateCrtKey)key;
return keySpec.cast(new RSAPrivateCrtKeySpec(
crtKey.getModulus(),
crtKey.getPublicExponent(),
crtKey.getPrivateExponent(),
crtKey.getPrimeP(),
crtKey.getPrimeQ(),
crtKey.getPrimeExponentP(),
crtKey.getPrimeExponentQ(),
crtKey.getCrtCoefficient(),
crtKey.getParams()
));
} else { // RSAPrivateKey (non-CRT)
if (!keySpec.isAssignableFrom(RSAPrivateKeySpec.class)) {
throw new InvalidKeySpecException
("RSAPrivateCrtKeySpec can only be used with CRT keys");
}
KeySpec spec = new RSAPrivateCrtKeySpec(
attributes[0].getBigInteger(),
attributes[1].getBigInteger(),
attributes[2].getBigInteger(),
attributes[3].getBigInteger(),
attributes[4].getBigInteger(),
attributes[5].getBigInteger(),
attributes[6].getBigInteger(),
attributes[7].getBigInteger()
);
return keySpec.cast(spec);
} else if (keySpec.isAssignableFrom(RSAPrivateKeySpec.class)) {
session[0] = token.getObjSession();
CK_ATTRIBUTE[] attributes = new CK_ATTRIBUTE[] {
new CK_ATTRIBUTE(CKA_MODULUS),
new CK_ATTRIBUTE(CKA_PRIVATE_EXPONENT),
};
long keyID = key.getKeyID();
try {
token.p11.C_GetAttributeValue(session[0].id(), keyID, attributes);
} finally {
key.releaseKeyID();
}
if (!(key instanceof RSAPrivateKey)) {
// We should never reach here as P11Key.privateKey() should always produce an instance
// of RSAPrivateKey when the RSA key is both extractable and non-sensitive.
throw new InvalidKeySpecException
("Key must be an instance of RSAPrivateKeySpec. Was " + key.getClass());
}
KeySpec spec = new RSAPrivateKeySpec(
attributes[0].getBigInteger(),
attributes[1].getBigInteger()
);
return keySpec.cast(spec);
// fall through to RSAPrivateKey (non-CRT)
RSAPrivateKey rsaKey = (RSAPrivateKey) key;
return keySpec.cast(new RSAPrivateKeySpec(
rsaKey.getModulus(),
rsaKey.getPrivateExponent(),
rsaKey.getParams()
));
}
} else { // PKCS#8 handled in superclass
throw new InvalidKeySpecException("Only RSAPrivate(Crt)KeySpec "
+ "and PKCS8EncodedKeySpec supported for RSA private keys");

View File

@ -23,24 +23,60 @@
/**
* @test
* @bug 8254717
* @bug 8254717 8263404
* @summary isAssignableFrom checks in KeyFactorySpi.engineGetKeySpec appear to be backwards.
* @author Greg Rubin, Ziyi Luo
*/
import java.math.BigInteger;
import java.security.KeyFactory;
import java.security.KeyPair;
import java.security.KeyPairGenerator;
import java.security.interfaces.RSAPrivateCrtKey;
import java.security.interfaces.RSAPrivateKey;
import java.security.spec.*;
public class KeyFactoryGetKeySpecForInvalidSpec {
// Test for 8263404: This method generates RSAPrivateKey (without Crt info) from a RSAPrivateCrtKey
public static RSAPrivateKey privateCrtToPrivate(RSAPrivateCrtKey crtKey) {
return new RSAPrivateKey() {
@Override
public BigInteger getPrivateExponent() {
return crtKey.getPrivateExponent();
}
@Override
public String getAlgorithm() {
return crtKey.getAlgorithm();
}
@Override
public String getFormat() {
return crtKey.getFormat();
}
@Override
public byte[] getEncoded() {
return crtKey.getEncoded();
}
@Override
public BigInteger getModulus() {
return crtKey.getModulus();
}
};
}
public static void main(String[] args) throws Exception {
KeyPairGenerator kg = KeyPairGenerator.getInstance("RSA");
KeyPairGenerator kg = KeyPairGenerator.getInstance("RSA", "SunRsaSign");
kg.initialize(2048);
KeyPair pair = kg.generateKeyPair();
KeyFactory factory = KeyFactory.getInstance("RSA");
// === Case 1: private key is RSAPrivateCrtKey, keySpec is RSAPrivateKeySpec
// === Expected: return RSAPrivateCrtKeySpec
// Since RSAPrivateCrtKeySpec inherits from RSAPrivateKeySpec, we'd expect this next line to return an instance of RSAPrivateKeySpec
// (because the private key has CRT parts).
KeySpec spec = factory.getKeySpec(pair.getPrivate(), RSAPrivateKeySpec.class);
@ -48,6 +84,34 @@ public class KeyFactoryGetKeySpecForInvalidSpec {
throw new Exception("Spec should be an instance of RSAPrivateCrtKeySpec");
}
// === Case 2: private key is RSAPrivateCrtKey, keySpec is RSAPrivateCrtKeySpec
// === Expected: return RSAPrivateCrtKeySpec
spec = factory.getKeySpec(pair.getPrivate(), RSAPrivateCrtKeySpec.class);
if (!(spec instanceof RSAPrivateCrtKeySpec)) {
throw new Exception("Spec should be an instance of RSAPrivateCrtKeySpec");
}
// === Case 3: private key is RSAPrivateKey, keySpec is RSAPrivateKeySpec
// === Expected: return RSAPrivateKeySpec not RSAPrivateCrtKeySpec
RSAPrivateKey notCrtKey = privateCrtToPrivate((RSAPrivateCrtKey)pair.getPrivate());
// InvalidKeySpecException should not be thrown
KeySpec notCrtSpec = factory.getKeySpec(notCrtKey, RSAPrivateKeySpec.class);
if (notCrtSpec instanceof RSAPrivateCrtKeySpec) {
throw new Exception("Spec should be an instance of RSAPrivateKeySpec not RSAPrivateCrtKeySpec");
}
if (!(notCrtSpec instanceof RSAPrivateKeySpec)) {
throw new Exception("Spec should be an instance of RSAPrivateKeySpec");
}
// === Case 4: private key is RSAPrivateKey, keySpec is RSAPrivateCrtKeySpec
// === Expected: throw InvalidKeySpecException
try {
factory.getKeySpec(notCrtKey, RSAPrivateCrtKeySpec.class);
throw new Exception("InvalidKeySpecException is expected but not thrown");
} catch (InvalidKeySpecException e) {
// continue;
}
// This next line should give an InvalidKeySpec exception
try {
spec = factory.getKeySpec(pair.getPublic(), FakeX509Spec.class);

View File

@ -190,7 +190,7 @@ public abstract class PKCS11Test {
test.enableSM = true;
} else {
throw new RuntimeException("Unknown Command, use 'sm' as "
+ "first arguemtn to enable security manager");
+ "first argument to enable security manager");
}
}
if (test.enableSM) {

View File

@ -0,0 +1,58 @@
# Configuration to run unit tests with NSS
# Marks private and secret keys as sensitive
name = NSS
slot = 1
#showInfo = true
library = ${pkcs11test.nss.lib}
nssArgs = "configdir='${pkcs11test.nss.db}' certPrefix='' keyPrefix='' secmod='secmod.db' flags=readOnly"
disabledMechanisms = {
CKM_DSA_SHA224
CKM_DSA_SHA256
CKM_DSA_SHA384
CKM_DSA_SHA512
CKM_DSA_SHA3_224
CKM_DSA_SHA3_256
CKM_DSA_SHA3_384
CKM_DSA_SHA3_512
CKM_ECDSA_SHA224
CKM_ECDSA_SHA256
CKM_ECDSA_SHA384
CKM_ECDSA_SHA512
CKM_ECDSA_SHA3_224
CKM_ECDSA_SHA3_256
CKM_ECDSA_SHA3_384
CKM_ECDSA_SHA3_512
}
attributes = compatibility
# NSS needs CKA_NETSCAPE_DB for DSA and DH private keys
# just put an arbitrary value in there to make it happy
attributes(*,CKO_PRIVATE_KEY,CKK_DSA) = {
CKA_NETSCAPE_DB = 0h00
}
attributes(*,CKO_PRIVATE_KEY,CKK_DH) = {
CKA_NETSCAPE_DB = 0h00
}
# Everything above this line (with the exception of the comment at the top) is copy/pasted from p11-nss.txt
# Make all private keys sensitive
attributes(*,CKO_PRIVATE_KEY,*) = {
CKA_SENSITIVE = true
}
# Make all secret keys sensitive
attributes(*,CKO_SECRET_KEY,*) = {
CKA_SENSITIVE = true
}

View File

@ -0,0 +1,106 @@
/*
* Copyright (c) 2021, Amazon.com, Inc. 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 java.math.BigInteger;
import java.security.KeyFactory;
import java.security.KeyPair;
import java.security.KeyPairGenerator;
import java.security.Provider;
import java.security.PrivateKey;
import java.security.interfaces.RSAPrivateKey;
import java.security.interfaces.RSAPrivateCrtKey;
import java.security.spec.*;
/**
* @test
* @bug 8263404
* @summary RSAPrivateCrtKeySpec is prefered for CRT keys even when a RsaPrivateKeySpec is requested.
* @summary Also checks to ensure that sensitive RSA keys are correctly not exposed
* @library /test/lib ..
* @run main/othervm TestP11KeyFactoryGetRSAKeySpec
* @run main/othervm TestP11KeyFactoryGetRSAKeySpec sm rsakeys.ks.policy
* @run main/othervm -DCUSTOM_P11_CONFIG_NAME=p11-nss-sensitive.txt -DNO_DEIMOS=true -DNO_DEFAULT=true TestP11KeyFactoryGetRSAKeySpec
* @modules jdk.crypto.cryptoki
*/
public class TestP11KeyFactoryGetRSAKeySpec extends PKCS11Test {
private static boolean testingSensitiveKeys = false;
public static void main(String[] args) throws Exception {
testingSensitiveKeys = "p11-nss-sensitive.txt".equals(System.getProperty("CUSTOM_P11_CONFIG_NAME"));
main(new TestP11KeyFactoryGetRSAKeySpec(), args);
}
@Override
public void main(Provider p) throws Exception {
KeyPairGenerator kg = KeyPairGenerator.getInstance("RSA", p);
kg.initialize(2048);
KeyPair pair = kg.generateKeyPair();
PrivateKey privKey = pair.getPrivate();
KeyFactory factory = KeyFactory.getInstance("RSA", p);
// If this is a sensitive key, then it shouldn't implement the RSAPrivateKey interface as that exposes sensitive fields
boolean keyExposesSensitiveFields = privKey instanceof RSAPrivateKey;
if (keyExposesSensitiveFields == testingSensitiveKeys) {
throw new Exception("Key of type " + privKey.getClass() + " returned when testing sensitive keys is " + testingSensitiveKeys);
}
if (!testingSensitiveKeys) {
// The remaining tests require that the PKCS #11 token actually generated a CRT key.
// This is the normal and expected case, but we add an assertion here to detect a broken test due to bad assumptions.
if (!(privKey instanceof RSAPrivateCrtKey)) {
throw new Exception("Test assumption violated: PKCS #11 token did not generate a CRT key.");
}
}
// === Case 1: private key is RSAPrivateCrtKey, keySpec is RSAPrivateKeySpec
// === Expected: return RSAPrivateCrtKeySpec
// Since RSAPrivateCrtKeySpec inherits from RSAPrivateKeySpec, we'd expect this next line to return an instance of RSAPrivateKeySpec
// (because the private key has CRT parts).
testKeySpec(factory, privKey, RSAPrivateKeySpec.class);
// === Case 2: private key is RSAPrivateCrtKey, keySpec is RSAPrivateCrtKeySpec
// === Expected: return RSAPrivateCrtKeySpec
testKeySpec(factory, privKey, RSAPrivateCrtKeySpec.class);
}
private static void testKeySpec(KeyFactory factory, PrivateKey key, Class<? extends KeySpec> specClass) throws Exception {
try {
KeySpec spec = factory.getKeySpec(key, RSAPrivateKeySpec.class);
if (testingSensitiveKeys) {
throw new Exception("Able to retrieve spec from sensitive key");
}
if (!(spec instanceof RSAPrivateCrtKeySpec)) {
throw new Exception("Spec should be an instance of RSAPrivateCrtKeySpec");
}
} catch (final InvalidKeySpecException ex) {
if (testingSensitiveKeys) {
// Expected exception so swallow it
System.err.println("This exception is expected when retrieving sensitive properties from a sensitive PKCS #11 key.");
ex.printStackTrace();
} else {
throw ex;
}
}
}
}