Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 8 commits into from

Conversation

ziyiluo
Copy link
Member

@ziyiluo ziyiluo commented Mar 11, 2021

This is a P2 regression introduced by JDK-8254717.

In RSAKeyFactory.engineGetKeySpec, when the key is a RSA key and the KeySpec is RSAPrivateKeySpec or RSAPrivateCrtKeySpec. The method behavior is described as follow:

X-axis: type of keySpec
Y-axis: type of key

Before JDK-8254717:

RSAPrivateKeySpec.class RSAPrivateCrtKeySpec.class
RSAPrivateKey Return RSAPrivateKeySpec Throw InvalidKeySpecException
RSAPrivateCrtKey Return RSAPrivateKeySpec Return RSAPrivateKeyCrtSpec

After JDK-8254717 (Green check is what we want to fix, red cross is the regression):

RSAPrivateKeySpec.class RSAPrivateCrtKeySpec.class
RSAPrivateKey Throw InvalidKeySpecException Throw InvalidKeySpecException
RSAPrivateCrtKey Return RSAPrivateKeyCrtSpec Return RSAPrivateKeyCrtSpec

This commit fixes the regression.

Tests

  • Jtreg: All tests under java/security, sun/security, javax/crypto passed
  • JCK: All JCK-16 (I do not have jCK-17)tests under api/java_security passed

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

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

Reviewers

Contributors

  • Greg Rubin <rubin@amazon.com>

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2949/head:pull/2949
$ git checkout pull/2949

To update a local copy of the PR:
$ git checkout pull/2949
$ git pull https://git.openjdk.java.net/jdk pull/2949/head

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 11, 2021

👋 Welcome back luoziyi! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Mar 11, 2021

@ziyiluo The following label will be automatically applied to this pull request:

  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added security security-dev@openjdk.org rfr Pull request is ready for review labels Mar 11, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 11, 2021

Webrevs

@wangweij
Copy link
Contributor

wangweij commented Mar 12, 2021

My understanding is that the problem here is the 2 isAssignableFrom checks are in wrong order. The parent class RSA_PRIV_KEYSPEC_CLS should be checked first.

BTW, please add a regression test to the fix. Thanks.

@SalusaSecondus
Copy link
Contributor

If we check the parent class RSA_PRIV_KEYSPEC_CLS first, then the JDK will return instances of RSAPrivateKeySpec even when it could return an instance of RSAPrivateCrtKeySpec. If the underlying RSA key has CRT parameters, then it seems preferable to return the more detailed and specific RSAPrivateCrtKeySpec if possible. (Not opportunistically returning RSAPrivateCrtKeySpec is actually one of the specific things that the prior change (which introduced this regression) is trying to fix.

@ziyiluo
Copy link
Member Author

ziyiluo commented Mar 12, 2021

Hi @wangweij Thanks for your review. As @SalusaSecondus commented, RSAPrivateKeyCrtSpec should be favored over RSAPrivateKeySpec when the PrivateKey is a Crt Key. I just modified our JTreg test to include all four cases described in the PR description.

@wangweij
Copy link
Contributor

I still cannot understand why CRT is always preferred. The original implementation also hadn't done that.

@SalusaSecondus
Copy link
Contributor

I believe that the original implementation intended to do this but made a mistake. This is why the original implementation (with the backwards isAssignableFrom logic) first checked to see if it could use CRT (as it had more information) and only afterwards fell back to seeing if it could use RSAPrivateKeySpec.

RSA CRT keys are much more efficient than normal RSA private keys and also result in more a more standard compliant output when serialized to PKCS#8 format (which technically requires the CRT parameters to be present). Thus, I believe we should try to preserve the CRT parameters whenever possible for our users. Now users who request an RSAPrivateKeySpec and then use it to later create a new key (using KeyFactory.generatePrivate) can keep the significant performance benefits for that private key.

@valeriepeng
Copy link

valeriepeng commented Mar 15, 2021

P11RSAKeyFactory.java should also be included into this fix as it's subject to the same RSAPrivateKeySpec vs RSAPrivateCrtKeySpec issue.

My personal take is that the isAssignableFrom() call may be more for checking if both keyspecs are the same. If the original impl (pre-8254717 code) really intend to return RSAPrivateCrtKeySpec for RSAPrivateCrtKey objs, then it should just check key type to be CRT and no need to call isAssignableFrom() with both RSAPrivateCrtKeySpec AND RSAPrivateKeySpec, just check the later would suffice. Thus, I'd argue the original intention is to return the keyspec matching the requested keyspec class.

I can see the potential performance benefit of piggybacking the Crt info and generating Crt keys when RSAPrivateKeySpec is specified. However, the way it is coded in this PR seems a bit unclear. The InvalidKeySpecException definitely must be fixed. As for the RSAPrivateKeySpec->RSAPrivateCrtKeySpec change, it's a change of behavior comparing to pre-8254717 code which I am ok with if we can be sure that there is no undesirable side-effects.

@SalusaSecondus
Copy link
Contributor

P11RSAKeyFactory.java should also be included into this fix as it's subject to the same RSAPrivateKeySpec vs RSAPrivateCrtKeySpec issue.

Thank you. We'll fix this.

My personal take is that the isAssignableFrom() call may be more for checking if both keyspecs are the same. If the original impl (pre-8254717 code) really intend to return RSAPrivateCrtKeySpec for RSAPrivateCrtKey objs, then it should just check key type to be CRT and no need to call isAssignableFrom() with both RSAPrivateCrtKeySpec AND RSAPrivateKeySpec, just check the later would suffice. Thus, I'd argue the original intention is to return the keyspec matching the requested keyspec class.

I wondered this too, but if that were the case, I'd expect to see RSA_PRIVCRT_KEYSPEC_CLS.equals(keySpec) instead. It seems more likely that the code is just trying to ensure that some valid implementation of the KeySpec is returned rather than the specific one.

I can see the potential performance benefit of piggybacking the Crt info and generating Crt keys when RSAPrivateKeySpec is specified. However, the way it is coded in this PR seems a bit unclear. The InvalidKeySpecException definitely must be fixed. As for the RSAPrivateKeySpec->RSAPrivateCrtKeySpec change, it's a change of behavior comparing to pre-8254717 code which I am ok with if we can be sure that there is no undesirable side-effects.

I'm definitely up for ways to make this clearer. I'll ensure that the next version has some better comments to explain the logic flow so that future us doesn't need to guess about original intent in the way we are now.

I agree that this should be a safe change. Outside of some extremely sensitive unit tests (which is how I discovered this bug in the first place), I don't expect much code to depend on the exact class returned by this method. (Any code doing so would be wrong as well, but we still need to pay attention to it.)

We can see similar behavior elsewhere in the JDK where a more specialized class is returned by a method similar to this.

        AlgorithmParameters params = AlgorithmParameters.getInstance("EC");
        params.init(new ECGenParameterSpec("secp256r1"));
        ECParameterSpec spec = params.getParameterSpec(ECParameterSpec.class);
        System.out.println(spec);
        System.out.println(spec.getClass());

In this case we can see that EcParameters returns an instance of the private class sun.security.util.NamedCurve specifically to preserve additional information about the parameters for later use within the JDK.

Copy link
Contributor

@SalusaSecondus SalusaSecondus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks to cover the cases and fixes we talked about.

@ziyiluo
Copy link
Member Author

ziyiluo commented Mar 18, 2021

@valeriepeng Sorry for the delay. There were unknown Windows build failure during the pre-submit tests that I have to rebase my commits on top of the master tip. This new revision should cover all comments you left before. Thank you!

Comment on lines 408 to 430
} else if (keySpec.isAssignableFrom(RSA_PRIVCRT_KEYSPEC_CLS) && 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 if (keySpec.isAssignableFrom(RSA_PRIV_KEYSPEC_CLS)) {
RSAPrivateKey rsaKey = (RSAPrivateKey)key;
return keySpec.cast(new RSAPrivateKeySpec(
rsaKey.getModulus(),
rsaKey.getPrivateExponent(),
rsaKey.getParams()
));
} else if (keySpec.isAssignableFrom(RSA_PRIVCRT_KEYSPEC_CLS)) {
throw new InvalidKeySpecException
("RSAPrivateCrtKeySpec can only be used with CRT keys");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are basing the decision on type of key, it may be clearer to do something like below:

        } else if (key instanceof RSAPrivateKey) {
            if (keySpec.isAssignableFrom(PKCS8_KEYSPEC_CLS)) {
                return keySpec.cast(new PKCS8EncodedKeySpec(key.getEncoded()));
            } else if (keySpec.isAssignableFrom(RSA_PRIVCRT_KEYSPEC_CLS)) {
                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(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 {
                throw new InvalidKeySpecException
                        ("KeySpec must be RSAPrivate(Crt)KeySpec or "
                        + "PKCS8EncodedKeySpec for RSA private keys");
            }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this flow and think that it's clearer.

One question: Do you think we could use this code almost character for character in the P11RSAKeyFactory? The keys handled should be descendants of RSAPrivateCrtKey, RSAPrivateKey, or P11PrivateKey (which would need some special handling). I didn't want to refactor it that much even though I think that it would work because I assume it must have been written to use the underlying PKCS#11 properties for some reason (even if I cannot figure out why).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, for P11RSAKeyFactory, I think some minor modification may be needed given the additional P11PrivateKey type.
I'd expect it to be something like:

        // must be either RSAPrivateKeySpec or RSAPrivateCrtKeySpec
        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);
                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);
            } catch (final PKCS11Exception ex) {
                // bubble this up if RSAPrivateCrtKeySpec is specified
                // otherwise fall through to RSAPrivateKeySpec
                if (!keySpec.isAssignableFrom(RSAPrivateKeySpec.class)) {
                    throw ex;
                }
            }  finally {
                key.releaseKeyID();
            }

            attributes = new CK_ATTRIBUTE[] {
                new CK_ATTRIBUTE(CKA_MODULUS),
                new CK_ATTRIBUTE(CKA_PRIVATE_EXPONENT),
            };
            keyID = key.getKeyID();
            try {
                token.p11.C_GetAttributeValue(session[0].id(), keyID, attributes);
            } finally {
                key.releaseKeyID();
            }

            KeySpec spec = new RSAPrivateKeySpec(
                attributes[0].getBigInteger(),
                attributes[1].getBigInteger()
            );
            return keySpec.cast(spec);
        } else { // PKCS#8 handled in superclass
            throw new InvalidKeySpecException("Only RSAPrivate(Crt)KeySpec "
                + "and PKCS8EncodedKeySpec supported for RSA private keys");
        }
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this code will work, but since there already is P11RSAPrivateKey and P11RSAPrivateNonCRTKey (both of which implement the standard RSAPrivateKey and RSAPrivateCrtKey interfaces respectively), can we just depend on and simplify our code? (My concern is that there is some unknown reason for calling C_GetAttributeValue directly here.)

For example (based on the non-P11 code above)

        } else if (key instanceof RSAPrivateKey) {
            if (keySpec.isAssignableFrom(PKCS8_KEYSPEC_CLS)) {
                return keySpec.cast(new PKCS8EncodedKeySpec(key.getEncoded()));
            } else if (keySpec.isAssignableFrom(RSA_PRIVCRT_KEYSPEC_CLS)) {
                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 if (key instanceof RSAPrivateKey) {
                    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 { // New P11 code starts here and handles the P11PrivateKey case
                    throw new InvalidKeySpecException("RSA Private key is not extractable");
                } // End new P11 code
            } else {
                throw new InvalidKeySpecException
                        ("KeySpec must be RSAPrivate(Crt)KeySpec or "
                        + "PKCS8EncodedKeySpec for RSA private keys");
            }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've used your code for both because it looks like a good solution.

One thing which puzzles me is that the following implementation for the P11 code passes all tests and avoids any interaction with P11 library at all. I didn't want to use it because I'm concerned that there is an untested edge-case we'd bump into.

    <T extends KeySpec> T implGetPrivateKeySpec(P11Key key, Class<T> keySpec,
            Session[] session) throws PKCS11Exception, InvalidKeySpecException {
        if (key instanceof RSAPrivateKey) {
            try {
                return implGetSoftwareFactory().getKeySpec(key, keySpec);
            } catch (final InvalidKeySpecException  e) {
                throw e;
            } catch (final GeneralSecurityException e) {
                throw new InvalidKeySpecException("Could not generate KeySpec", e);
            }
        } else {
            throw new InvalidKeySpecException("Key is not extractable");
        }
    }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, when the key object has all the attribute values available, things will often work regardless which path you took.

@mlbridge
Copy link

mlbridge bot commented Mar 19, 2021

Mailing list message from Michael StJohns on security-dev:

On 3/19/2021 2:24 PM, Valerie Peng wrote:

some* reason (even if I cannot figure out why).
Well, for `P11RSAKeyFactory`, I think some minor modification may be needed given the additional P11PrivateKey type.
I'd expect it to be something like:
// must be either RSAPrivateKeySpec or RSAPrivateCrtKeySpec
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),

If the PKCS11 private key has the CKA_SENSITIVE attribute set to true or
CKA_EXPORTABLE set to false, you can't retrieve the above attribute.?
AIRC, the contract for getting a Key from an unextractable PKCS11
private key is to return a key that implements both PrivateKey and
RSAKey, but doesn't implement either of the RSAPrivateKey interfaces.??
I don't know what the contract is for producing KeySpec's from
unextractable keys.

Mike

@SalusaSecondus
Copy link
Contributor

Mike,

From what I can find, if you try to get a spec from a non-extractable key you'll get an InvalidKeySpecException.

  1. C_GetAttributeValuewill throw a PKCS11Exception
  2. The PKCS11Exception gets caught in P11KeyFactory which rethrows it as an InvalidKeySpecException.

@mlbridge
Copy link

mlbridge bot commented Mar 20, 2021

Mailing list message from Michael StJohns on security-dev:

On 3/20/2021 1:54 PM, SalusaSecondus wrote:

Given that, I'd refactor the code to pull the CKA_SENSITIVE and
CKA_EXPORTABLE attributes first and throw a more specific message if the
key is not extractable rather than having to fail twice before throwing
the error.? (I.e., you try both combos of the attributes and both are
failing on the inability to pull the private exponent).

Either that or fail early by checking the error code of the first thrown
PKCS11Exception against CKR_ATTRIBUTE_SENSITIVE.

  \} catch \(final PKCS11Exception ex\) \{

if (ex.getErrorCode() == PKCS11Constants.CKR_ATTRIBUTE_SENSITIVE) {
???? throw new InvalidKeySpecException ("Sensitive key may not be
extracted", ex);
}

             \/\/ bubble this up if RSAPrivateCrtKeySpec is specified
             \/\/ otherwise fall through to RSAPrivateKeySpec
             if \(\!keySpec\.isAssignableFrom\(RSAPrivateKeySpec\.class\)\) \{
                 throw ex\;
             \}
         \}  finally \{
             key\.releaseKeyID\(\)\;
         \}

Later, Mike

@SalusaSecondus
Copy link
Contributor

We seem to have a choice and I'm not sure the best way to approach this.

  1. We trust the properties in P11Key and just ask it if the values are both sensitive and extractable. [1]
  2. But if we already trust P11Key, why not also trust that it properly implements the RSAPrivateKey interfaces [2]. This is the strategy used by the snippet I posted earlier (delegating to implGetSoftwareFactory())
  3. We don't trust P11Key except to use getKeyId(), this yields the current design where we pull the attributes every time the factory needs them.

We should probably reduce calls to C_GetAttributeValue as they may be very slow. At the least they cross the JNI boundary and at worst they interact with a slow piece of hardware (possibly over a network). The current design will have two calls in a worst case, but is likely to have only one call the vast majority of the time.

[1] https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java#L92
[2] https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java#L375-L406

@mlbridge
Copy link

mlbridge bot commented Mar 20, 2021

Mailing list message from Michael StJohns on security-dev:

On 3/20/2021 2:46 PM, SalusaSecondus wrote:

Actually, the important lines are 63-66 and 365-373:

?* If the components are not accessible, we use a generic class that
?* only implements PrivateKey (or SecretKey). Whether the components of a
?* key are extractable is automatically determined when the key object is
?* created.

attributes = getAttributes(session, keyID, attributes, new
CK_ATTRIBUTE[] {
??????????? new CK_ATTRIBUTE(CKA_TOKEN),
??????????? new CK_ATTRIBUTE(CKA_SENSITIVE),
??????????? new CK_ATTRIBUTE(CKA_EXTRACTABLE),
??????? });
??????? if (attributes[1].getBoolean() || (attributes[2].getBoolean()
== false)) {
??????????? return new P11PrivateKey
??????????????? (session, keyID, algorithm, keyLength, attributes);
??????? }

If the key is non-extractable, then the only attributes will be these
three and the underlying type will be P11Key.P11PrivateKey rather than
one of the RSA variants.

Simple check at the top.

Mike

@SalusaSecondus
Copy link
Contributor

P11PrivateKey is private so we cannot check that. Our options to figure out if something is sensitive are:

  1. See if it doesn't implement RSAPrivateKey (this yields the prior snippet with implGetSoftwareFactory())
  2. Try to access the attributes directly from the token (this yields the current solution which we're modifying)
  3. Check the value of p11Key.extractable (which is package-private and thus visible)

The smallest change would be to keep our strategy as 2. While I like it the least (my favorite is number 1) it has the smallest risk of changing some undocumented behavior on a PKCS#11 device that we're unfamiliar with (and not testing because we may not have the hardware costing tens of thousands of dollars). Option 3 somewhat spits the difference between 1 and 2.

@openjdk openjdk bot removed the rfr Pull request is ready for review label Mar 24, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 24, 2021
}
// 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 retrive the attributes we need. This sub-class will also cache these attributes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: retrieve

Comment on lines 90 to 91
// Expected exception so swallow it
ex.printStackTrace();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a System.out.println() to clarify that this exception stack trace is expected?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting it in System.err.println() so that it goes to the same output as the stack trace.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, you know what I mean.

@@ -26,23 +26,27 @@
import java.security.KeyPair;
import java.security.KeyPairGenerator;
import java.security.Provider;
import java.security.interfaces.RSAPrivateCrtKey;
import java.security.PrivateKey;
import java.security.interfaces.RSAPrivateKey;
import java.security.spec.*;

/**
* @test
* @bug 8263404
* @summary RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be clearer to re-word the summary to clear things up, e.g. return RSAPrivateCrtKeySpec for CRT Keys even when RSAPrivateKeySpec is specified for KeyFactory.getKeySpec() calls.

Comment on lines 85 to 87
if (!(spec instanceof RSAPrivateCrtKeySpec)) {
throw new Exception("Spec should be an instance of RSAPrivateCrtKeySpec");
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that SunPKCS11 provider does not really generate the keys unlike SunRsaSign provider, thus for correctness, you should check it's a CRT key before you impose the instanceof RSAPrivateCrtKeySpec check.

import java.security.interfaces.RSAPrivateKey;
import java.security.spec.*;

/**
* @test
* @bug 8263404
* @summary RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec
* @summary Also checks to ensure that sensitive RSA keys are correctly not exposed
* @author Greg Rubin, Ziyi Luo

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest policy is to not include @author tag.

Comment on lines +15 to +32
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
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is to avoid using this special provider for non-RSA impls? Add a comment?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is copy/pasted from p11-nss.txt with only the sensitive modifications at the end. I do not know why those disabled mechanisms are present in the source file.

@valeriepeng
Copy link

Rest of changes look good. Thanks for the update.

@openjdk
Copy link

openjdk bot commented Mar 26, 2021

@ziyiluo This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

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

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 161 new commits pushed to the master branch:

  • c9d2d02: 8263632: Improve exception handling of APIs in classLoader.cpp
  • 59ed1fa: 8264087: Use the blessed modifier order in jdk.jconsole
  • 054e0a4: 8264017: Correctly report inlined frame in JFR sampling
  • d6bb153: 8264240: [macos_aarch64] enable appcds support after JDK-8263002
  • 7284f01: 8262110: DST starts from incorrect time in 2038
  • 3a28dc8: 8264178: Unused method Threads::nmethods_do
  • 33c94ff: 8263376: CTW (Shenandoah): assert(mems <= 1) failed: No node right after call if multiple mem projections
  • 4e74de4: 8264111: (fs) Leaking NativeBuffers in case of errors during UnixUserDefinedFileAttributeView.read/write
  • 57115fa: 8189198: Add "forRemoval = true" to Applet API deprecations
  • b8122d6: 8264220: jdk/javadoc/doclet/testRelatedPackages/TestRelatedPackages.java fails to compile
  • ... and 151 more: https://git.openjdk.java.net/jdk/compare/2b93ae00194d034ca906fd7731f436fec430a09d...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@valeriepeng) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 26, 2021
@ziyiluo
Copy link
Member Author

ziyiluo commented Mar 26, 2021

@valeriepeng Thanks for the review. Do we need a second reviewer for this PR? Besides, are you willing to sponsor this?

@ziyiluo
Copy link
Member Author

ziyiluo commented Mar 26, 2021

/contributor add Greg Rubin rubin@amazon.com

@openjdk
Copy link

openjdk bot commented Mar 26, 2021

@ziyiluo
Contributor Greg Rubin <rubin@amazon.com> successfully added.

@valeriepeng
Copy link

I can sponsor this. It looks like you need to do the "/integrate" comment first?

@ziyiluo
Copy link
Member Author

ziyiluo commented Mar 26, 2021

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 26, 2021
@openjdk
Copy link

openjdk bot commented Mar 26, 2021

@ziyiluo
Your change (at version 4ec6ff3) is now ready to be sponsored by a Committer.

@valeriepeng
Copy link

/sponsor

@openjdk openjdk bot closed this Mar 29, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 29, 2021
@openjdk
Copy link

openjdk bot commented Mar 29, 2021

@valeriepeng @ziyiluo Since your change was applied there have been 182 commits pushed to the master branch:

  • 128c0c9: 8248418: jpackage fails to extract main class and version from app module linked in external runtime
  • fd45694: 8264344: Outdated links in JavaComponentAccessibility.m
  • f17ea9e: 8262899: TestRedirectLinks fails
  • 963f1fc: 8264309: JFR: Improve .jfc parser
  • 364cce1: 8264332: Use the blessed modifier order in jdk.charsets
  • fbbd98b: 8264029: Replace uses of StringBuffer with StringBuilder in java.base
  • 019080e: 8264268: Don't use oop types for derived pointers
  • 3516c26: 8263591: Two C2 compiler phases with the name "after matching"
  • 3caea47: 8262739: String inflation C2 intrinsic prevents insertion of anti-dependencies
  • 19a6ac4: 8264142: Remove TRAPS/THREAD parameters for verifier related functions
  • ... and 172 more: https://git.openjdk.java.net/jdk/compare/2b93ae00194d034ca906fd7731f436fec430a09d...master

Your commit was automatically rebased without conflicts.

Pushed as commit a5d7de2.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@ziyiluo ziyiluo deleted the 8263404 branch March 29, 2021 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated security security-dev@openjdk.org
4 participants