On Mon, 4 Jan 2021 21:35:48 GMT, Valerie Peng <valer...@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
