-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8271566: DSA signature length value is not accurate in P11Signature #4961
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
Conversation
👋 Welcome back mbalao! A progress list of the required criteria for merging this PR into |
Webrevs
|
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java
Outdated
Show resolved
Hide resolved
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java
Outdated
Show resolved
Hide resolved
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java
Outdated
Show resolved
Hide resolved
As for your changes to P11Key class, I wonder why NSS doesn't make the "extractable" attribute false for their token keys? I agree that this change makes sense for NSS given their current impl but not sure how other vendor may be impacted by this. So, how about:
|
Hmm, I ran more security regression tests against the proposed changes and had a second thought about this proposed P11Key change. When the underlying P11 DSA key is un-extractable and returned as a P11Key which implements DSAPrivateKey interface. With the proposed change, calling getX() upon this key object returns null which will lead to unexpected NPE. This is a serious problem. Same goes for other private keys, e.g. EC, DH. And not just the private value of these keys, but also the encodings may lead to NPE. Thus, for un-extractable keys, we will have to continue to return them as P11PrivateKey objects which does not implement any algorithm specific interface. |
Yes, I see what you mean. Contrary to P11PrivateKey::getFormat and P11PrivateKey::getEncodedInternal where a 'null' returned value is documented in java.security.Key, we don't have that documentation for the other interfaces such as java.security.interfaces.DSAPrivateKey. That can lead to NPE if the client casts the P11Key to the interface, invokes the method and depends on a non-null value. I will give this some thought and try to come up with something, because the information is already there and (in reality) we need it internally only. It's clear that all these interfaces were not designed with unextractable P11 keys in mind, because it makes sense to me (conceptually) to have a private key from which we can retrieve some values (public, such as the parameters) and not other ones. |
Right, PKCS11 and unextractable keys come a few releases later after the JCA API. So, there may be some difficulties trying to work them into existing APIs. In order to maintain backward compatibility, API changes are also limited. |
I made some class refactoring in P11Key to achieve the following goals:
The refactorings implied removing fields from private but serializable classes. In example, P11DSAPublicKey does not longer have the fields 'y' and 'params'. My understanding is that this a serial compatibility breaker and a CSR would be needed. I can address that but wish you could have a look at the proposal before, so we come to something stable first. No test regressions observed in jdk/sun/security/pkcs11 category. |
I agree with the new class hierarchy and see the merits. But need to think more about this new Fetcher way of doing things. It seems that the Fetcher is the real key class where there is the common logic (sensitive, isPrivate, etc.) and you just cover it up with different wrapper classes which seems algorithm specific but yet do not contain any algorithm specific fields and the logic is all inside the corresponding Fetcher. In addition, there seems to be some duplicated code in retrieving the attributes in RSA*Fetcher vs P11Key.fetchAttributes(). Will try a few things before sharing my review comments. Thanks. Valerie |
I tried a few things out and prefer just minor refactoring over this new Fetcher mechanism. I have shared my prototype changes with you over email. Please check it out. Thanks. Valerie |
@martinuy 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! |
@martinuy This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
/open |
@martinuy This pull request is now open |
Hi Valerie, Apologies for the delay; I've intention to resume this work if you can. I had a look at your Webrev.00 but I'm not really convinced, at least when comparing it to the Fetcher approach. I'll share my comments below:
From comment #Aug 17:
If you still want to take the non-fetcher approach and address the issues mentioned above, I can have a look. Kind regards, |
Hmm, ok, I was focus on something else and missed your update to this PR. it's been a while, I will take another look to refresh my memory and get back to you... Thanks~ |
Hi Martin, Please find my comments in line below.
Hmm, good point. I agree that returning null as default format and override it whenever an encoding is returned is the right thing to do.
Sure, I didn't spend much time on this. Either Opaque or Internal suffix is fine. Generally I prefer shorter names.
This is where our view differs. We both have same class hierarchy but where the attributes are managed are different, you put all of them in the very bottom, i.e. fetcher, but I prefer them to be at top. Using RSA as an example, there are P11RSAPrivateKeyInternal - sensitive private key
There shouldn't be another call to the native library as it is only made when the modulus n is null. However, since n is already available, overriding the getModulus() method makes sense as there is no need to call fetchValues() which should return upon a non-null n value, but still an overhead.
The proposed P11RSAPrivateKeyInternal class has no method for returning the public exponent either. If needed, it should be doable by adding extra attribute to the list of attributes in P11PrivateKeyRSA class. Should be trivial. I updated the class naming in my prototype to align with yours and updated the prototype changes with your feedback. You can check/compare it at: http://cr.openjdk.java.net/~valeriep/8271566/webrev.01/ We have different views on this I guess. I prefer to let each class manage their list of attributes instead of a separate fetcher with their own logic. Former also has less code and follows the same model of existing code. Thanks, |
Hi @valeriepeng , Thanks for having a look at this. While we might have slightly different views on the fetcher, I appreciate that you took the time to explain your reasons. In any case, I believe that this iteration will be a significant improvement when compared to the old code, so I'm good to move forward on that. I'll check the other discussion points and your Webrev.01 proposal as soon as possible. Martin.- |
In my view (Webrev.00 based comment), the variable 'n' holding the modulus value is private in P11RSAPrivateKey. This means that when P11PrivateKeyRSA::getModulus is called, P11PrivateKeyRSA::n (which is protected) has a 'null' value and the PKCS#11 lib call is done again. |
Hmm, this is a bug and unintended. The 'n' in the child class should be removed as the 'n' in the parent class has scope protected and should be used instead. I checked webrev.01 and this has been caught and fixed already. Good to have a different pair of eyes and more likely to spot problems. Thanks! Regards, |
Hi @valeriepeng , Some comments and questions regarding Webrev.01:
Thanks, |
Either internal or opaque suffix works for me. Fields aren't really being written out into the serialized bytes are "transient". IIRC, the serialized bytes are the "encoding" instead of the fields, we should mark these fields transient. As for the P11RSAPrivateNonCRTKey, yes, it should use the 'n' from parent instead of its own. I missed it when working on webrev.01. As for 'long errorCode = e.getErrorCode();' in P11Signature.java, yes, it should have been removed. I was manually merging the changes when refreshing the source tree and missed to remove this line. Thanks, |
Hmm, thinking more about "internal"/"opaque", given this is naming for the parent, maybe "internal" is more correct. The non-sensitive keys are encapsulated by the children classes and is still an instance of the parent. If you name the parent class w/ "opaque" suffix, it looks misleading/confusing. The opaqueness is implied by the implementation instead of the properties of the objects. |
Hi @valeriepeng , Yes, I just verified how serialization works for P11Keys and your 'transient' change makes sense to me now. I see your point about Internal/Opaque. Keep 'Internal' then if you prefer. The whole inheritance relationship between these classes sounds a bit weird to me, beyond the names we call them. I wouldn't suggest any additional changes there now, though. It looks to me that the only 2 changes expected for Webrev.02 are: 1) P11RSAPrivateNonCRTKey to use the parent's protected 'n'; and 2) removal of 'long errorCode = e.getErrorCode();'. Now that we almost have the changeset ready, I'm not sure of how to proceed with the push. Do you want me to commit Webrev.02 in my own branch and use the 'Co-authored-by:' header? If we do that, do we still need an additional Reviewer? Do you prefer to have your own branch? Please let me know of how to move forward. Martin.- |
You can just incorporate the changes on your branch and proceeds with me being the reviewer. The webrev that I sent u is just an easier way to express the comments/feedbacks. Thanks, |
Hi @valeriepeng I've reverted the initial 2 changesets on this branch, rebased to the latest commit on 'master' (git merge), applied Webrev.02 as discussed and added the test case from the initial commit (with the change from the 2nd commit applied on top). I've noticed no regressions in jdk/sun/security/pkcs11. Does it look good to you? Thanks, |
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java
Outdated
Show resolved
Hide resolved
Hi @valeriepeng , Please take a look at the latest change based on your comments. No regressions found in 'jdk/sun/security/pkcs11'. Thanks, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks! Valerie
@martinuy 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:
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 56 new commits pushed to the
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. ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit ea8d3c9.
Your commit was automatically rebased without conflicts. |
As described in JDK-8271566 [1], this patch proposal is intended to fix a problem that arises when using DSA keys that have a 256-bits (or larger) G parameter for signatures (either signing or verifying). There were some incorrect assumptions and hard-coded length values in the code before. Please note that, for example, the tuple (2048, 256) for DSA is valid according to FIPS PUB 186-4.
Beyond the specific issues in signatures, I decided to provide a broader solution and enable key parameter retrieval for other key types (EC, DH) when possible. This is, when the key is not sensitive. One thing that I should note here is that token keys (those that have the CKA_TOKEN attribute equal to 'true') are considered sensitive in this regard, at least by the NSS Software Token implementation. I don't have access to other vendor implementations but if there is any concern, we can adjust the constraint to NSS-only. However, I'm not sure which use-case would require to get private keys out of a real token, weakening its security. I'd be more conservative here and not query the values if not sure that it will succeed.
No regressions found in jdk/sun/security/pkcs11. A new test added: LargerDSAKey.
--
[1] - https://bugs.openjdk.java.net/browse/JDK-8271566
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4961/head:pull/4961
$ git checkout pull/4961
Update a local copy of the PR:
$ git checkout pull/4961
$ git pull https://git.openjdk.java.net/jdk pull/4961/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4961
View PR using the GUI difftool:
$ git pr show -t 4961
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4961.diff