8258833: Cancel multi-part cipher operations in SunPKCS11 after failures #1901
Conversation
|
👋 Welcome back mbalao! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
Thanks for finding this! There have been intermittent CKR_OPERATION_ACTIVE errors which is likely the result of this bug... Existing SunPKCS11 code is based on the PKCS#11 spec, e.g. a call to C_EncryptUpdate which results in an error other than CKR_BUFFER_TOO_SMALL terminates the current encryption operation, and thus avoid the canceling operation for such scenarios. Ideally, NSS should have properly terminated the operation as the spec described. Knowing the NSS behavior, this defensive-cancellation fix is good. There are additional SunPKCS11 impl classes which follow the same model as P11Cipher class and may have to be updated with this defensive-cancellation fix as well. How about P11AEADCipher.java, P11RSACipher.java, P11Signature.java, P11PSSSignature.java, P11Mac.java? Thanks, |
| cipher.update(new byte[1]); | ||
| cipher.doFinal(new byte[1], 0, 0); | ||
| } else { | ||
| cipher.update(new byte[1]); |
There was a problem hiding this comment.
Why only calling update(..) for Cipher encryption would lead to Exception? Seems strange...
There was a problem hiding this comment.
Because a C_EncryptUpdate call that returns with an error here [1] implies that a session (with an active operation) is returned to the Session Manager here [2] [3]. For decryption, where we have proper padding on the Java side while doing an update, the test exercises the doFinal path. Decryption/Encryption is anecdotal here: what the test wants is coverage on both update and doFinal paths.
--
[1] -
[2] -
[3] -
There was a problem hiding this comment.
I understand that the intention/design is to trigger the error condition on various code paths causing the active session to be returned to pool and verify if this issue is fixed or not. What I don't get is that why AES/ECB/NoPadding cipher encrypting a byte[1] w/ an update (where more input could still be given) would trigger an error. The stack trace for the encryption update call (which is the first test, the session should be freshly allocated and not a reused one with active state) is below:
java.security.ProviderException: update() failed
at jdk.crypto.cryptoki/sun.security.pkcs11.P11Cipher.implUpdate(P11Cipher.java:632)
at jdk.crypto.cryptoki/sun.security.pkcs11.P11Cipher.engineUpdate(P11Cipher.java:529)
at jdk.crypto.cryptoki/sun.security.pkcs11.P11Cipher.engineUpdate(P11Cipher.java:517)
at java.base/javax.crypto.Cipher.update(Cipher.java:1832)
at CancelMultipart$LeakByteArray.doOperation(CancelMultipart.java:125)
at CancelMultipart$SessionLeaker.leakAndTry(CancelMultipart.java:63)
at CancelMultipart.executeTest(CancelMultipart.java:158)
at CancelMultipart.main(CancelMultipart.java:140)
at PKCS11Test.premain(PKCS11Test.java:171)
at PKCS11Test.testNSS(PKCS11Test.java:568)
at PKCS11Test.main(PKCS11Test.java:207)
at CancelMultipart.main(CancelMultipart.java:131)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:567)
at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
at java.base/java.lang.Thread.run(Thread.java:831)
Caused by: sun.security.pkcs11.wrapper.PKCS11Exception: CKR_DATA_LEN_RANGE
at jdk.crypto.cryptoki/sun.security.pkcs11.wrapper.PKCS11.C_EncryptUpdate(Native Method)
at jdk.crypto.cryptoki/sun.security.pkcs11.P11Cipher.implUpdate(P11Cipher.java:584)
... 17 more
There was a problem hiding this comment.
The update fails because the native mechanism (CKM_AES_ECB) has no padding and OpenJDK does not buffer data in the Java side for encryption [1] (this is a bug that I'll address soon). As a result, there is a PKCS#11 call with an invalid length and we get the error that ends up returning the session to the Session Manager. I just realized that when we fix the previous padding-bug, this test path won't work anymore. CKR_BUFFER_TOO_SMALL errors on updates do not lead to a reset call in the OpenJDK side (contrary to doFinal), so they wouldn't be useful for the test. I'll investigate if there is a way to trigger the path. Otherwise we should keep the doFinal path only. I'd still force a reset if there is an error other than CKR_BUFFER_TOO_SMALL in the update.
--
[1] -
There was a problem hiding this comment.
It's an update call, isn't padding occur when doFinal() is called for encryption?
In any case, it's best for the test case to not have this bug dependency. I am ok if you can only test doFinal path only.
There was a problem hiding this comment.
Yes, makes sense to remove the bug dependency and the whole encrypt-update path. I'll keep the test extensible, though; so we can include new paths eventually.
| key = new SecretKeySpec(new byte[16], "AES"); | ||
| } | ||
|
|
||
| private static class SessionLeaker { |
There was a problem hiding this comment.
The term "leak/leaker" is used extensively in the test, however, this is not really a leak conceptually, but rather sessions w/ active states/operations which are unusable and lead to unexpected PKCS#11 errors subsequently. Maybe replace it with other terms like "corrupt/corruptor" or other similar terms would be more suitable?
There was a problem hiding this comment.
I chose the term 'leak' in the sense of a resource not properly cleaned up. In this case, the 'leak' would not cause a memory/sockets/file-descriptors but a 'usable-sessions' exhaustion. It's always an analogy, but the sense is that something (a Session) unexpectedly (with an active operation) passes from one side (a P11Cipher instance) to another (the Session Manager). I don't believe 'corruptor' describes the concept better than 'leaker'. The Session is not corrupt, it can be used for for a specific purpose (the same operation that previously failed). Any other suggestion? Hmm... I cannot think of something better.
There was a problem hiding this comment.
So, after this PKCS11 session failed with an error, you can continue using it, say give it another byte[] and it would continue to work as if the earlier failed call never happened? I have not really tried it and thought that this session is unusable due to the combination of an existing error (say some exception happened during the encrypt update call) and active operation state.
There was a problem hiding this comment.
Yes, that's right: you can use the session to continue the same operation. In fact, there is a pattern -also used in some Windows API functions- that you send an output buffer of length 0, you get a CKR_BUFFER_TOO_SMALL error and the buffer size that would have been required; and you make a second call with the right buffer size. This pattern is used from the OpenJDK native library that communicates with the PKCS#11 lib. Some types of errors (those that do not free the context) are not fatal. The problem is that we end up returning a session to the Session Manager and we try to use the session to initialize a different operation, while the previous is still active.
There was a problem hiding this comment.
The double-call-query usage pattern is well documented in the PKCS#11 spec. So, there is no ambiguity there. As for what PKCS11 errors are fatal and the resulting session state seems to be a gray area. Anyway, I agree that defensive cancelling this to work with all PKCS11 library is good so to ensure subsequent operations using the reused session work.
I can't think of other better terms. Fine with me to just use "leak" w/ your analogy explanation.
|
Thanks for having a look at this. You're right: there might be problems beyond P11Cipher and this is a good opportunity to have a look at them. I'll start with an analysis of P11Signature. P11Signature makes the following PKCS#11 calls: C_SignInit, C_VerifyInit, C_SignFinal, C_Sign, C_VerifyFinal, C_Verify, C_SignUpdate and C_VerifyUpdate. The type of bug described in JDK-8258833 can potentially happen in all of them except for C_SignInit and C_VerifyInit, where the operation does not need to be terminated upon a failure. In the NSS Software Token (checked on NSS 3.50), an NSC_SignUpdate call goes to sftk_MACUpdate with the 'type' parameter equal to 'SFTK_SIGN' [1]. Assumming there is a 'context' for the type that can be retrieved (otherwise CKR_OPERATION_ACTIVE cannot occur [2]), there are only two paths in which sftk_MACUpdate can return an error (in other words, crv != CKR_OK): [3] and [4]. For these two paths, execution goes to sftk_TerminateOp [5] [6]; which finally cleans up the context [7]. This is consistent with sftk_MACUpdate documentation here [8]. Thus, I don't expect the same kind of error that we may have for NSC_EncryptUpdate. Note how in NSC_EncryptUpdate there are no calls to sftk_TerminateOp [9]. Forcing a 'cancel' operation from OpenJDK when calling C_SignUpdate [10] [11] would incurr in an unnecessary cost; but this cost is paid on an already-slow path. The same reasoning applies to NSC_VerifyUpdate, as it uses same sftk_MACUpdate function with the 'type' parameter equal to 'SFTK_SIGN' [12]. When it comes to C_SignFinal and C_VerifyFinal; the operation (assumming a valid context was obtained) is almost always terminated [13] [14]. There is an exception to the previous statement: In C_SignFinal, a CKR_BUFFER_TOO_SMALL does not terminate the session (while returning an error) [15]. This is PKCS#11 standard compliant, and must be handled in the OpenJDK side. The approach in P11Cipher to analogous cases is to throw an exception and cancel the operation at the P11Cipher-level (returning a session with an active operation to the Session Manager previous to JDK-8258833 fix) [16] [17]. The current P11Signature implementation does not do this [18], and incurrs in the bug of returning the session to the SessionManager with an active operation. We need to fix this in every place where C_SignFinal is used. In C_Sign, functions such as NSC_SignUpdate and NSC_SignFinal (which handle active operations in most cases if an error occurs) are used for multi-part operations. If it's not a multi-part operation and the error is different than CKR_BUFFER_TOO_SMALL, the operation is terminated [19]. This is similar to what happens in NSC_SignFinal, and we need to handle it in the OpenJDK side every time C_Sign is called (i.e.: [20] [21]). C_Verify either uses NSC_VerifyUpdate/NSC_VerifyFinal or always terminates the operation [22]; so it shouldn't be a problem. In summary, I believe we need changes in the OpenJDK side to properly handle CKR_BUFFER_TOO_SMALL errors when C_SignFinal or C_Sign PKCS#11 functions are called from P11Signature. Even if other error types or functions such as C_VerifyFinal, C_Verify, NSC_SignUpdate and NSC_VerifyUpdate should not need an explicit cancel; we might want to do it anyways to be more defensive and do not depend on a specific NSS implementation. -- [11] - [12] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L3558 [13] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L3597 [14] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L3095 [15] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L3088 [16] - [17] - [18] - https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java#L657 [19] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L3145 [20] - https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java#L636 [21] - https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java#L642 [22] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L3541 |
I'm not entirely sure that we can trigger the CKR_BUFFER_TOO_SMALL potential bug from OpenJDK because the output buffer is allocated in the OpenJDK native code [1] for C_Sign and the length is equal to 4K [2]. In the case of C_SignFinal, the CKR_BUFFER_TOO_SMALL error is handled in native code [3]. My understanding is that we still want to be defensive here, and do not depend on a specific NSS version that may leak active operations on some types of errors. The difference is that this change in P11Signature won't have a regression test. @valeriepeng are you okay with this reasoning? -- |
I changed my mind around this decision and I'm inclined not to make any code changes to P11Signature. Only a documentation note that reflects this analysis should be needed. First of all, what I described here [1], about the NSS Software Token behavior, matches the PKCS#11 v2.20 standard [2]:
In addition, as described here [3], CKR_BUFFER_TOO_SMALL is not expected to reach P11Signature Java code because it's handled by the libj2pkcs11 OpenJDK native library. Thus, cancelling a potentially active operation is not required by the standard nor needed. If we find a non-compliant implementation of the PKCS#11 standard in the future, we might want to revisit this decision. Even if the performance cost of cancelling an operation 'just in case' should be affordable, we might unnecessarily get into errors such as CKR_OPERATION_NOT_INITIALIZED. The P11Cipher case is different because the size of the output buffer (the one that may lead to a CKR_BUFFER_TOO_SMALL error) is a user input and the error visible to OpenJDK Java code [4] [5] [6] [7]. In addition, and contrary to the PKCS#11 v2.20 standard -which states "A call to C_EncryptUpdate which results in an error other than CKR_BUFFER_TOO_SMALL terminates the current encryption operation."-, the NSS Software Token may not terminate the operation on other error types [8] [9]. This is why we need to always cancel from P11Cipher. -- [5] - [6] - [7] - [8] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L1356 [9] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L1380 |
|
Sure, the reason that I brought up about other classes besides P11Cipher is to confirm/check whether NSS impl conforms to the PKCS#11 spec for all these calls where the same cancellation pattern is used. If NSS impl follows the PKCS#11 spec, that's fine and it's safer to not change the existing code. |
For cipher impls, there are more than just P11Cipher, there are also P11AEADCipher and P11RSACipher. It looks like they should be updated with this defensive cancellation change unless the non-compliant NSS impl is algorithm-specific and does not apply to AES/GCM and RSA. |
Sure, I was going to go through each of them. Only P11Cipher and P11Signature so far but I'm working on this. |
Wonderful, thanks! |
…; only C_DecryptUpdate/C_EncryptUpdate ones.
…tation improvements.
… when the operation being cancelled was not initialized.
P11PSSSignatureThe P11PSSSignature case is analogous to PSSSignature, because the PKCS#11 primitives and their use from OpenJDK are similar. Added a note explaining why doCancel = false is safe even after C_Sign or C_SignFinal (so anyone reading the PKCS#11 standard and wondering about cancellation after CKR_BUFFER_TOO_SMALL errors or successful calls to determine the output length will have an answer there). In summary, this is handled at libj2pkcs native level. The only theoretical way I see to get an error here is that the output of the signature is greater than MAX_STACK_BUFFER_LEN -which, of course, will cause more critical issues-. Added a check in cancelOperation, so we have a consistent behavior with other P11 services. Cancel Operation should not fail if the operation being cancelled was not initialized. P11AEADCipherIn P11AEADCipher C_EncryptUpdate/C_DecryptUpdate/C_EncryptFinal/C_DecryptFinal PKCS#11 calls are not used but C_Encrypt/C_Decrypt only. According to the PKCS#11 standard [1], C_Encrypt does not cancel the operation in these cases:
The NSS Software Token implementation seems to honor this specification in NSC_Encrypt [2]. These paths are not triggerable from OpenJDK because the output length is checked against an expected value before making the PKCS#11 call [3]. So it should be safe to assume that the operation will always be terminated in P11AEADCipher::implDoFinal. As a result, I conclude that no code changes are require in P11AEADCipher::implDoFinal but a documentation note explaining why doCancel = false is safe there [4]. The C_Decrypt case is analogous to C_Encrypt. Added a check in cancelOperation as in P11PSSSignature. P11RSACipherC_WrapKey/C_UnwrapKey/C_GenerateKey: these calls are atomic, instead of multi-part operations; so there shouldn't be an issue there. In the case of C_Encrypt, C_Decrypt, C_Sign and C_VerifyRecover (P11RSACipher::implDoFinal), a reset with doCancel = true is not done. C_Encrypt/C_Decrypt can theoretically keep an operation active as listed for P11AEADCipher. However, I was unable to find in the NSS Software Token a path leading to a CKR_BUFFER_TOO_SMALL error from P11RSACipher::implDoFinal (CKM_RSA_PKCS or CKM_RSA_X_509 schemes): if the output buffer length is too short, we get a CKR_DEVICE_ERROR error coming from here [5]. I was unable to trigger a successful call with output length equal to 0 (to get the output length from the library) because direct buffers are not used [6] -see argument #6-, length-0 buffers do not return a null pointer here [7] (which is checked here [8] in the NSS side) and null buffers do not make the PKCS#11 call [9]. Looks to me that C_Encrypt may return CKR_BUFFER_TOO_SMALL for some algorithms such as RSA-OAEP, but that's not what P11RSACipher is currently using. C_Sign may return CKR_BUFFER_TOO_SMALL from NSC_SignFinal. However, the user does seem to have enough influence on the PKCS#11 call here [10] to trigger this path. I don't have evidence that the bug could be triggered in P11RSACipher; but the static analysis I've done is limited/error-prone. I'm inclined not to make any change unless we have real evidence that a cancel is required. Added a check in cancelOperation as in P11PSSSignature. P11MacNot cancelling from OpenJDK when C_SignFinal is called is safe as described before in this thread. In the case of a C_SignUpdate error, P11Mac::reset is not called so the Session is not returned to the Session Manager. Added a check in cancelOperation as in P11PSSSignature. P11CipherRevisiting the P11Cipher case, I realised that the bug can be triggered from C_EncryptUpdate/C_DecryptUpdate calls only and not from C_EncryptFinal/C_DecryptFinal. Thus, in P11Cipher::implDoFinal we might be cancelling when not necessary. Note that P11Cipher::implDoFinal may perform a C_EncryptUpdate/C_DecryptUpdate call internally. Fixed that: do not cancel as a result of a C_EncryptFinal/C_DecryptFinal failure. -- [4] - [5] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/freebl/rsapkcs.c#L911 [6] - [7] - [8] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L1463 [9] - [10] - |
|
@valeriepeng let me know your thoughts. Nothing else from my side now, unless you want me to revisit something. |
…ends on a know bug to cause a PKCS#11 error.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java
Show resolved
Hide resolved
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java
Show resolved
Hide resolved
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java
Show resolved
Hide resolved
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java
Show resolved
Hide resolved
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java
Show resolved
Hide resolved
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java
Show resolved
Hide resolved
|
@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 344 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 |
…pher::implDoFinal(ByteBuffer..'. Better documentation in P11Cipher. Copyright date updated.
| // Some implementations such as the NSS Software Token do not | ||
| // cancel the operation upon a C_EncryptUpdate failure (as | ||
| // required by the PKCS#11 standard). Cancel is not needed | ||
| // only after this point. See JDK-8258833 for further | ||
| // information. | ||
| doCancel = false; |
There was a problem hiding this comment.
@valeriepeng I made a code change here so I'd like you to have a final look and validate. I'm just aligning the 'P11Cipher::implDoFinal(byte[]..' function to 'P11Cipher::implDoFinal(ByteBuffer..'. The rationale is that 'doFalse = false' can be placed before the C_EncryptFinal call because any error on it does not require a cancel (it already cancels the operation)
There was a problem hiding this comment.
Yes, aligning them is better.
| // required by the PKCS#11 standard). Cancel is not needed | ||
| // only after this point. See JDK-8258833 for further | ||
| // information. | ||
| doCancel = false; |
There was a problem hiding this comment.
Same comment that for line 793 of P11Cipher
| k -= actualPadLen; | ||
| System.arraycopy(padBuffer, 0, out, outOfs, k); | ||
| } else { | ||
| doCancel = false; |
There was a problem hiding this comment.
Same comment than for line 793 of P11Cipher
|
/integrate |
openjdk
bot
added
integrated
|
@martinuy Since your change was applied there have been 385 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 47c7dc7. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
2 participants
When a multi-part cipher operation fails in SunPKCS11 (i.e. because of an invalid block size), we now cancel the operation before returning the underlying Session to the Session Manager. This allows to use the returned Session for a different purpose. Otherwise, an CKR_OPERATION_ACTIVE error would be raised from the PKCS#11 library.
The jdk/sun/security/pkcs11/Cipher/CancelMultipart.java regression test is introduced as part of this PR.
No regressions found in jdk/sun/security/pkcs11.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/1901/head:pull/1901$ git checkout pull/1901