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

[Valerie Peng]

On Fri, 8 Jan 2021 19:28:55 GMT, Martin Balao <mbalao at openjdk.org> wrote:

>>> 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.
>>> 
>> 
>> 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?
>> 
>> --
>> [1] - https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_sign.c#L142
>> [2] - https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/native/libj2pkcs11/pkcs11wrapper.h#L166
>> [3] - https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_sign.c#L252
>
>> 
>> @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]:
> 
>  1) "A call to C_SignFinal always terminates the active signing operation unless it returns CKR_BUFFER_TOO_SMALL or is a successful call"; and,
> 
>  2) "A call to C_Sign always terminates the active signing operation unless it returns CKR_BUFFER_TOO_SMALL".
> 
> 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.
> 
> --
> [1] - https://git.openjdk.java.net/jdk/pull/1901#issuecomment-756312031
> [2] - https://www.cryptsoft.com/pkcs11doc/v220/group__SEC__11__11__SIGNING__AND__MACING__FUNCTIONS.html
> [3] - https://git.openjdk.java.net/jdk/pull/1901#issuecomment-756376546
> [4] - https://github.com/openjdk/jdk/blob/cd94606c0c2dbf0a7f6d08dcc27f787ed080ac15/src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_crypt.c#L280
> [5] - https://github.com/openjdk/jdk/blob/cd94606c0c2dbf0a7f6d08dcc27f787ed080ac15/src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_crypt.c#L228
> [6] - https://github.com/openjdk/jdk/blob/cd94606c0c2dbf0a7f6d08dcc27f787ed080ac15/src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_crypt.c#L463
> [7] - https://github.com/openjdk/jdk/blob/cd94606c0c2dbf0a7f6d08dcc27f787ed080ac15/src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_crypt.c#L514
> [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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1901