RH2173781: Avoid calling C_GetInfo() too early, before cryptoki is initialized #26
Conversation
|
Hi @fridrich, there is an ongoing updated and more polished upstream proposal of our #2 patch: openjdk#12396. We plan to backport it to @martinuy, I think you did that change analogous to @fridrich's one, do you remember what was going on? I seem to recall this being discovered by running the SunPKCS11 tests. Regarding |
|
I am fine with your solution. I did this submit request just to avoid that the patch gets lost in my rpm packages. And yes, declaring the exception as thrown is valid solution. I had a choice between that and catching it, and I chose the other part. But since you people have the solution to this problem, I will kill this PR. |
|
Hi @fridrich, Thanks for reporting this issue and sending a fix proposal. Even when the code in JDK head should address this issue of calling C_GetInfo too early —there is a PKCS11Exception catch block to let execution continue—, we'd be grateful if you can provide more details. In particular, we want to know what is the PKCS 11 token that you are using and what is the error that you get (at the native level). We couldn't reproduce with the NSS Software Token. I'm reopening this PR. Thanks, |
|
Actually, I did not see it myself, but one customer of mine got natively CKR_CRYPTOKI_NOT_INITIALIZED in the constructor. So, I just read the diff and decided not to call that C_GetInfo in constructor, but later on, when executing the getInfo method the first time. Since the original code had the C_GetInfo calls not wrapped by anything and it was working for the customer. The catching of the exception was just what I chose between the two options, since I knew that the original code was not throwing. Now, declaring the exception as thrown in the getInfo is at the second thought the best solution, since the C_GetInfo was also throwing it in the original code. So, catching it was my mistake. Which I will change in the patch. |
|
But, the fact is that I could not reproduce it either, but reading the customer's error and reading the code around those places made me come with something that is similar to what the JDK head now has. But the original code with the C_GetInfo in constructor passes even all the JCK tests here, so does my modified (not so correct though) code. And we use NSS softok too. |
|
Hi @fridrich , Thanks for providing more details. You're right in that CKR_CRYPTOKI_NOT_INITIALIZED can be returned as part of calling C_GetInfo [1], which translates into a PKCS11Exception that was not previously thrown in the PKCS11 constructor. I have some concerns regarding hiding this exception in a try-catch block —as done for JDK head—, but we should keep alignment to head in 17u anyways and, if there is something to fix or improve, do it in head first. Thus, we will apply a temporary fix here, in the lines of what you proposed but considering some thread-safety issues. When the PBE proposal for OpenJDK upstream is accepted in head and backported to 17u, we will probably go with a slightly different fix. @franferrax what do you think? Martin.- -- |
|
Actually, the customer trace was: |
|
The reproducer is: |
On second thought, I also think that it is a mistake. If one wanted to avoid zillion of calls to C_GetInfo, at least the first invocation should throw if a native error occurs. I just went with the first solution that was working for the customer :) |
|
@martinuy: I agree with your proposal for a temporary fix, let's use this same pull request so we all review and agree on that short-term fix. |
…zation in some PKCS #11 tokens
|
@franferrax @fridrich, please have a look at my last commit. I added some thread-safety code in getInfo because we no longer have the guarantee that it's called from the constructor when pInfo is assigned. I handle the case in which getInfo returns null in P11SecretKeyFactory.java. This case, however, looks unlikely or impossible to me because initializing a SunPKCS11 instance requires a non-null CK_INFO. Anyways, this won't be available in the future fix because CK_VERSION —which will be used directly instead of CK_INFO— won't be null. |
|
Bugzilla tracking: https://bugzilla.redhat.com/show_bug.cgi?id=2173781 |
The commit looks good. I was just wondering whether it was not better to leave the previous code instead of having to make the getInfo thread-safe. Not sure how many times was the C_GetInfo called. For sure having the assignment in constructor was a real gain, but how much the gain is now, I am uncertain. |
franferrax
left a comment
franferrax
left a comment
There was a problem hiding this comment.
In reply to @fridrich's comment:
The commit looks good. I was just wondering whether it was not better to leave the previous code instead of having to make the getInfo thread-safe. Not sure how many times was the C_GetInfo called. For sure having the assignment in constructor was a real gain, but how much the gain is now, I am uncertain.
The reason why we decided to cache C_GetInfo()'s result is that the original code wasn't calling it as frequently as after our changes for RH2048582: Support PKCS#12 keystores. I don't think calling C_GetInfo() in that place would have a catastrophic impact on performance (it is one more JNI call when deriving PBE keys, where several of them are involved), but I also don't dislike @martinuy's currently proposed fix.
Taking in mind that this will be handled in a cleaner way for the upstream jdk proposal, eventually back-ported to upstream jdk17u, and finally removed from RHEL downstream FIPS patches, current code in this PR looks good to me.
gnu-andrew
changed the title
4 participants
Search this PR in Red Hat Jira
The call of C_GetInfo() in the PKCS11 class constructor is too early, since that function needs cryptoki initialized. This patch fixes this issue. And it still uses the pInfo structure instead of calling C_GetInfo zillion times.
I think that the fix is right. At least it solved here issues I saw. But I might be utterly wrong ;)