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

Reviewed-by: simonis, mbalao, andrew
Backport-of: a5d7de235101696463dba22792703c6809ff7fc4

Author: Paul Hohensee

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

@@ -282,54 +282,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");

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

@@ -421,6 +421,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(
@@ -434,17 +435,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 "

jdk/test/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);

jdk/test/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
+}

jdk/test/sun/security/pkcs11/rsa/TestP11KeyFactoryGetRSAKeySpec.java

@@ -0,0 +1,105 @@
+/*
+ * 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 /lib/testlibrary ..
+ * @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
+ */
+
+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;
+            }
+        }
+    }
+}