On Wed, 13 Jan 2021 00:53:01 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>>> 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. > >> >> >> > 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! P11PSSSignature --------------------- The 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. P11AEADCipher --------------------- In 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: 1) CKR_BUFFER_TOO_SMALL error; and, 2) Successful call to determine the length of the output buffer. 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. P11RSACipher --------------------- C_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. P11Mac --------------------- Not 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. P11Cipher --------------------- Revisiting 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. -- [1] - https://www.cryptsoft.com/pkcs11doc/v220/pkcs11__all_8h.html#aC_Encrypt [2] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L1437 [3] - https://github.com/openjdk/jdk/blob/1cc09ccaef9a3695dd2862e3ee121e141e0a8a13/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java#L588 [4] - https://github.com/openjdk/jdk/blob/1cc09ccaef9a3695dd2862e3ee121e141e0a8a13/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java#L619 [5] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/freebl/rsapkcs.c#L911 [6] - https://github.com/openjdk/jdk/blob/1cc09ccaef9a3695dd2862e3ee121e141e0a8a13/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11RSACipher.java#L278 [7] - https://github.com/openjdk/jdk/blob/1cc09ccaef9a3695dd2862e3ee121e141e0a8a13/src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_crypt.c#L156 [8] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L1463 [9] - https://github.com/openjdk/jdk/blob/1cc09ccaef9a3695dd2862e3ee121e141e0a8a13/src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_crypt.c#L158 [10] - https://github.com/openjdk/jdk/blob/1cc09ccaef9a3695dd2862e3ee121e141e0a8a13/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11RSACipher.java#L382 ------------- PR: https://git.openjdk.java.net/jdk/pull/1901
