RFR: 8258833: Cancel multi-part cipher operations in SunPKCS11 after failures

Fri Jan 15 18:39:25 UTC 2021

On Wed, 13 Jan 2021 00:53:01 GMT, Valerie Peng <valeriep at 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