On Fri, 8 Jan 2021 19:28:55 GMT, Martin Balao <mba...@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
