8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySp…

…ec in RSAKeyFactory.engineGetKeySpec

Co-authored-by: Greg Rubin <rubin@amazon.com>
Reviewed-by: valeriep

src/java.base/share/classes/sun/security/rsa/RSAKeyFactory.java

@@ -406,6 +406,7 @@ protected <T extends KeySpec> T engineGetKeySpec(Key key, Class<T> keySpec)
             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 @@ protected <T extends KeySpec> T engineGetKeySpec(Key key, Class<T> keySpec)
                         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 "

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

@@ -277,54 +277,51 @@ <T extends KeySpec> T implGetPublicKeySpec(P11Key key, Class<T> keySpec,
 
     <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");

test/jdk/java/security/KeyFactory/KeyFactoryGetKeySpecForInvalidSpec.java

@@ -23,31 +23,95 @@
 
 /**
  * @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);
         if (!(spec instanceof RSAPrivateCrtKeySpec)) {
             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);

test/jdk/sun/security/pkcs11/PKCS11Test.java

@@ -190,7 +190,7 @@ public static void main(PKCS11Test test, String[] args) throws Exception {
                     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) {

test/jdk/sun/security/pkcs11/nss/p11-nss-sensitive.txt

@@ -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
+}