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

Thu Jan 7 18:59:00 UTC 2021

On Mon, 4 Jan 2021 21:35:48 GMT, Valerie Peng <valeriep at openjdk.org> wrote:

>> 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.
>
> 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,
> Valerie

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.

--
[1] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L3041
[2] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L423
[3] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L3010
[4] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L3015
[5] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L3027
[6] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L393
[7] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L401
[8] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L2973
[9] - https://github.com/nss-dev/nss/blob/561b982921f15902088a3f05b1cdf561f5f64cb7/lib/softoken/pkcs11c.c#L1303
[10] - https://github.com/openjdk/jdk/blob/78be334c3817a1b5840922a9bf1339a40dcc5185/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java#L526
[11] - https://github.com/openjdk/jdk/blob/78be334c3817a1b5840922a9bf1339a40dcc5185/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java#L573
[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] - https://github.com/openjdk/jdk/blob/1cc09ccaef9a3695dd2862e3ee121e141e0a8a13/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java#L803
[17] - https://github.com/openjdk/jdk/blob/1cc09ccaef9a3695dd2862e3ee121e141e0a8a13/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java#L898
[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

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

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