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

8271199: Mutual TLS handshake fails signing client certificate with custom sensitive PKCS11 key #4887

Closed
wants to merge 5 commits into from

Conversation

@alexeybakhtin
Copy link
Contributor

@alexeybakhtin alexeybakhtin commented Jul 23, 2021

Hello,

Could you please review the small patch for the issue described in JDK-8271199: Mutual TLS handshake fails signing client certificate with custom sensitive PKCS11 key

I suggest updating the RSAPSSSignature.isValid() method to verify if provided key components can be applied to SunRSASign implementation.
If not applied, implementation can try to select signer from other providers

Regards
Alexey


Progress

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

Issue

  • JDK-8271199: Mutual TLS handshake fails signing client certificate with custom sensitive PKCS11 key

Reviewers

Reviewing

Using git

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

Update a local copy of the PR:
$ git checkout pull/4887
$ git pull https://git.openjdk.java.net/jdk pull/4887/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4887

View PR using the GUI difftool:
$ git pr show -t 4887

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4887.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 23, 2021

👋 Welcome back abakhtin! 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jul 23, 2021

@alexeybakhtin 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.

Loading

@openjdk openjdk bot added the security label Jul 23, 2021
@openjdk openjdk bot added the rfr label Jul 23, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 23, 2021

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 20, 2021

@alexeybakhtin This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Loading

@alexeybakhtin
Copy link
Contributor Author

@alexeybakhtin alexeybakhtin commented Aug 20, 2021

Gentle ping

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 17, 2021

@alexeybakhtin This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Loading

@BenEfrati
Copy link

@BenEfrati BenEfrati commented Oct 3, 2021

Hey,
Same issue using nCipherKM provider

Caused by: com.ncipher.provider.nCSecurityException: The key material is not exportable outside of the HSM
        at com.ncipher.provider.Utils.exportKey(Utils.java:1539)
        at com.ncipher.provider.Utils.getCrtParams(Utils.java:1486)
        at com.ncipher.provider.km.KMRSAPrivateCrtKey.getPrimeP(KMRSAPrivateCrtKey.java:87)
        at sun.security.rsa.RSACore.crtCrypt(RSACore.java:168)
        at sun.security.rsa.RSACore.rsa(RSACore.java:122)
        at sun.security.rsa.RSAPSSSignature.engineSign(RSAPSSSignature.java:371)
        at java.security.Signature.engineSign(Signature.java:1382)
        at java.security.Signature.sign(Signature.java:698)
        at sun.security.ssl.CertificateVerify.<init>(CertificateVerify.java:608)
        at sun.security.ssl.CertificateVerify.produce(CertificateVerify.java:760)
        at sun.security.ssl.SSLHandshake.produce(SSLHandshake.java:421)
        at sun.security.ssl.ServerHelloDone.consume(ServerHelloDone.java:182)
        at sun.security.ssl.SSLHandshake.consume(SSLHandshake.java:377)
        at sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:444)
        at sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:422)
        at sun.security.ssl.TransportContext.dispatch(TransportContext.java:182)
        at sun.security.ssl.SSLTransport.decode(SSLTransport.java:149)
        at sun.security.ssl.SSLSocketImpl.decode(SSLSocketImpl.java:1143)
        at sun.security.ssl.SSLSocketImpl.readHandshakeRecord(SSLSocketImpl.java:1054)
        at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:394)
        

Is this fix is planned for OpenJDK 8u312?

Loading

@alexeybakhtin
Copy link
Contributor Author

@alexeybakhtin alexeybakhtin commented Oct 4, 2021

Is this fix is planned for OpenJDK 8u312?
Hi @BenEfrati, No chance to go to OpenJDK 8u312. Maybe 8u322 if it will be approved

Loading

@yarix
Copy link

@yarix yarix commented Oct 4, 2021

Hi @yarix, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user yarix for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

Loading

Copy link
Member

@XueleiFan XueleiFan left a comment

It's a good catch. Thank you for the fix.

Loading

if (rsaKey.getModulus().signum() == 0 ||
(rsaKey instanceof RSAPrivateKey rsaPrKey &&
(rsaPrKey.getPrivateExponent().signum() == 0 ||
(rsaPrKey instanceof RSAPrivateCrtKey crtKey &&
(crtKey.getPrimeP().signum() == 0 ||
crtKey.getPrimeQ().signum() == 0 ||
crtKey.getPrimeExponentP().signum() == 0 ||
crtKey.getPrimeExponentQ().signum() == 0 ||
crtKey.getCrtCoefficient().signum() == 0 ||
crtKey.getPublicExponent().signum() == 0 )))) ||
(rsaKey instanceof RSAPublicKey rsaPubKey &&
rsaPubKey.getPublicExponent().signum() == 0)) {
throw new InvalidKeyException("Invalid key attributes");
}
Copy link
Member

@XueleiFan XueleiFan Oct 20, 2021

Choose a reason for hiding this comment

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

What's the motivation to check the signum?

Loading

crtKey.getPrimeExponentQ().signum() == 0 ||
crtKey.getCrtCoefficient().signum() == 0 ||
crtKey.getPublicExponent().signum() == 0 )))) ||
(rsaKey instanceof RSAPublicKey rsaPubKey &&
Copy link
Member

@XueleiFan XueleiFan Oct 20, 2021

Choose a reason for hiding this comment

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

Mixing the public key and private key together in one method may be not straightforward enough to logics like this update. What do you think it we have two isvalid() method, one for private key and one for public key?

Loading

crtKey.getPublicExponent().signum() == 0 )))) ||
(rsaKey instanceof RSAPublicKey rsaPubKey &&
rsaPubKey.getPublicExponent().signum() == 0)) {
throw new InvalidKeyException("Invalid key attributes");
Copy link
Member

@XueleiFan XueleiFan Oct 20, 2021

Choose a reason for hiding this comment

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

The exception description may be confusing to users. I'm not sure if the checking could be simplified and make this exception message better matching the problems.

Loading

@alexeybakhtin
Copy link
Contributor Author

@alexeybakhtin alexeybakhtin commented Oct 21, 2021

Hi @XueleiFan !
Thank you for review and comments. I've used signum as it is the most lightweight method in the BigInteger. Now I have refactored the patch to reuse key verification methods from other classes.
isValid() is split out into two methods as you suggested and exception messages are updated.
sun/security/rsa tests are passed

Loading

@@ -123,12 +123,13 @@ public RSAPSSSignature() {
@Override
protected void engineInitVerify(PublicKey publicKey)
throws InvalidKeyException {
if (!(publicKey instanceof RSAPublicKey)) {
if (publicKey instanceof RSAPublicKey rsaPubKey) {
this.pubKey = (RSAPublicKey) isPublicKeyValid(rsaPubKey);
Copy link
Member

@XueleiFan XueleiFan Oct 21, 2021

Choose a reason for hiding this comment

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

Is it possible to have the isPublicKeyValid() returning RSAPublicKey, or void, so as to avoid the casting? Same comment for the isValid() method and the isPrivateKeyValid() method.

Loading

(key.getCrtCoefficient().signum() == 0)) {
try {
return isValid(key);
} catch (InvalidKeyException ikEx) {
Copy link
Member

@XueleiFan XueleiFan Oct 21, 2021

Choose a reason for hiding this comment

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

It might be an uncommon use of the exception. Exception creation and catching are expensive. It is not good for performance to recover from exception. I know there are a lot of code recovering from exception. But if it is possible, we may want to try avoid it in new code. Maybe, the isValid() could return a boolean value instead.

Loading

@alexeybakhtin
Copy link
Contributor Author

@alexeybakhtin alexeybakhtin commented Oct 21, 2021

Thank you for review.
I've changed exception handling and excluded unnecessary casting.

Loading

Copy link

@valeriepeng valeriepeng left a comment

Changes look fine. Have you run through all security regression tests besides those under sun/security/rsa?

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Oct 22, 2021

@alexeybakhtin 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:

8271199: Mutual TLS handshake fails signing client certificate with custom sensitive PKCS11 key

Reviewed-by: xuelei, 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 1060 new commits pushed to the master branch:

  • 6523c55: 8198336: java/awt/FontMetrics/FontCrash.java fails in headless mode
  • 88bbf3c: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
  • 4dec8fc: 8275645: [JVMCI] avoid unaligned volatile reads on AArch64
  • 4e647aa: 8275416: G1: remove unnecessary make_referent_alive in precleaning phase
  • dd622e5: 8275783: G1: fix incorrect region type documentation in HeapRegionType
  • b2128a9: 8263155: Allow additional contents for DMG
  • 1efe946: 8275712: Hashtable literal_size functions are broken
  • fab3d6c: 8275761: Backout: JDK-8274794 Print all owned locks in hs_err file
  • c978ca8: 8275344: -Xcheck:jni produces some warnings in the LCMS.c
  • 6a466fe: 8202056: Expand serial warning to check for bad overloads of serial-related methods and ineffectual fields
  • ... and 1050 more: https://git.openjdk.java.net/jdk/compare/ea182b5c6eba5f7e66b324068d3b42ffb2d29190...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 (@XueleiFan, @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).

Loading

@openjdk openjdk bot added the ready label Oct 22, 2021
* Validate the specified RSAPrivateKey
*/
private void isPrivateKeyValid(RSAPrivateKey prKey) throws InvalidKeyException {
InvalidKeyException ikException = null;
Copy link
Member

@XueleiFan XueleiFan Oct 22, 2021

Choose a reason for hiding this comment

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

If I read the code correct, by define a local variable, it looks like you are trying to avoid to re-throw the exception in the catch clause. But this style adds additional effort to read the code. Exception re-throw should be fine as if the exception has been generated and will be thrown in the end of the method.

Loading

crtKey.getModulus().bitLength(),
crtKey.getPublicExponent());
} else {
ikException = new InvalidKeyException(
Copy link
Member

@XueleiFan XueleiFan Oct 22, 2021

Choose a reason for hiding this comment

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

See above comment, I will just throw the exception, rather than cache it.

Loading

@alexeybakhtin
Copy link
Contributor Author

@alexeybakhtin alexeybakhtin commented Oct 22, 2021

Agree, It looks less readable. Just tried to avoid rethrowing the exception. Reverted back to the original version.

Loading

@alexeybakhtin
Copy link
Contributor Author

@alexeybakhtin alexeybakhtin commented Oct 22, 2021

Changes look fine. Have you run through all security regression tests besides those under sun/security/rsa?

Hi @valeriepeng,
Yes, I've run this code with all security tests. Passed successfully.

Loading

Copy link
Member

@XueleiFan XueleiFan left a comment

It looks good to me. Please make sure to run all security regression tests. Thank you!

Loading

@alexeybakhtin
Copy link
Contributor Author

@alexeybakhtin alexeybakhtin commented Oct 22, 2021

Thank you all. :jdk_security tests passed without regression

Loading

@alexeybakhtin
Copy link
Contributor Author

@alexeybakhtin alexeybakhtin commented Oct 22, 2021

/integrate

Loading

@openjdk openjdk bot added the sponsor label Oct 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 22, 2021

@alexeybakhtin
Your change (at version ede6436) is now ready to be sponsored by a Committer.

Loading

@yan-too
Copy link

@yan-too yan-too commented Oct 25, 2021

/sponsor

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Oct 25, 2021

Going to push as commit f623298.
Since your change was applied there have been 1065 commits pushed to the master branch:

  • 1da5e6b: 8275104: IR framework does not handle client VM builds correctly
  • 5dab76b: 8275381: G1: refactor 2 constructors of G1CardSetConfiguration
  • 5bbe4ca: 8275293: A change done with JDK-8268764 mismatches the java.rmi.server.ObjID.hashCode spec
  • c94dc2a: 8272854: split runtime/CommandLine/PrintTouchedMethods.java test
  • fec470f: 8272163: Add -version option to keytool and jarsigner
  • 6523c55: 8198336: java/awt/FontMetrics/FontCrash.java fails in headless mode
  • 88bbf3c: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
  • 4dec8fc: 8275645: [JVMCI] avoid unaligned volatile reads on AArch64
  • 4e647aa: 8275416: G1: remove unnecessary make_referent_alive in precleaning phase
  • dd622e5: 8275783: G1: fix incorrect region type documentation in HeapRegionType
  • ... and 1055 more: https://git.openjdk.java.net/jdk/compare/ea182b5c6eba5f7e66b324068d3b42ffb2d29190...master

Your commit was automatically rebased without conflicts.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Oct 25, 2021

@yan-too @alexeybakhtin Pushed as commit f623298.

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants