On Fri, 24 May 2024 11:42:37 GMT, Francisco Ferrari Bihurriet <fferr...@openjdk.org> wrote:
>> Hi, >> >> I would like to propose an implementation to support AES CBC with Ciphertext >> Stealing (CTS) in SunPKCS11, according to what has been specified in >> [JDK-8330843 CSR](https://bugs.openjdk.org/browse/JDK-8330843). >> >> What follows are implementation notes that describe the most relevant >> aspects of the proposed change. >> >> ### Buffering >> >> A buffering mechanism was implemented so PKCS #11 tokens only receive >> multipart input updates (`C_EncryptUpdate` or `C_DecryptUpdate`) in >> multiples of the block size. This mechanism protects against tokens —such as >> NSS— that assume an update with data not multiple of the block size is final >> and do variant ordering between the last 2 blocks, returning an incorrect >> partial output and resetting state. For example, when encrypting, a token >> may receive an update with an input not multiple of the block size and >> prematurely finalize the operation returning the last two blocks of >> ciphertext according to its ordering. By buffering on the Java side, the >> token is not aware of the end of the stream during updates and SunPKCS11 >> retains full control to do the last two blocks at `C_EncryptFinal` or >> `C_DecryptFinal`. A bug in NSS (see >> [1823875](https://bugzilla.mozilla.org/show_bug.cgi?id=1823875#c2)) requires >> buffering three blocks instead of two. If this bug gets fixed, the three >> block b uffering will still work and allow it to support old versions of the NSS library. >> >> ### `implUpdate` implementation >> >> The code added to `P11Cipher::implUpdate` follows the existing three-stage >> strategy: 1) Process data in the `padBuffer`, 2) Process data in the input >> and 3) Buffer to `padBuffer`. Stage #1 for CTS has some additional >> complexity that is worth describing in this section. >> >> The stage begins by calculating the total data available (what is already >> buffered in `padBuffer` + the new input) and assigning this value to the >> variable `totalInLen`. `newPadBufferLen` is the number of bytes that must be >> buffered at the end of the update operation, irrespective of where they come >> from (`padBuffer` or input). This number of bytes is determined out of >> `totalInLen` and based on the fact that each operation must retain bytes >> from the last two —or three, for NSS— blocks in `padBuffer`, or retain >> whatever is available if less than that. `dataForP11Update` is the number of >> bytes to be passed to the token during the operation (`C_EncryptUpdate` or >> `C_DecryptUpdate` calls), irrespective of the stage in which they are passed >> and of the dat... > > Francisco Ferrari Bihurriet has updated the pull request with a new target > base due to a merge or a rebase. The incremental webrev excludes the > unrelated changes brought in by the merge/rebase. The pull request contains > seven additional commits since the last revision: > > - Revert re-arrangement of native methods parameters > > This reverts commit 0a777e94229723376e1264e87cbf0ba805dc736f, except for > the copyright which is retained as 2024. > > NOTE: new calls of the same methods are retained in the re-arrangement > style, as we didn't introduce this re-arrangement, it was already > present in most of the calls inside ::implUpdate() and ::implDoFinal(). > > Co-authored-by: Francisco Ferrari <fferr...@redhat.com> > Co-authored-by: Martin Balao <mba...@redhat.com> > - Merge 'openjdk/master' into JDK-8330843 > - 8330842: Add AES CBC with Ciphertext Stealing (CTS) SunPKCS11 tests > > Co-authored-by: Francisco Ferrari <fferr...@redhat.com> > Co-authored-by: Martin Balao <mba...@redhat.com> > - 8330842: Support AES CBC with Ciphertext Stealing (CTS) in SunPKCS11 > > Co-authored-by: Francisco Ferrari <fferr...@redhat.com> > Co-authored-by: Martin Balao <mba...@redhat.com> > - Fix cipher tests logging > > Co-authored-by: Francisco Ferrari <fferr...@redhat.com> > Co-authored-by: Martin Balao <mba...@redhat.com> > - Implement integer constants as enum > > Co-authored-by: Francisco Ferrari <fferr...@redhat.com> > Co-authored-by: Martin Balao <mba...@redhat.com> > - Arrange parameters of native methods evenly > > C_EncryptUpdate / C_DecryptUpdate / C_EncryptFinal / C_DecryptFinal > > If the call doesn't fit on a single line, use the following order: > long hSession, > [ long directIn, byte[] in, int inOfs, int inLen, ] > long directOut, byte[] out, int outOfs, int outLen > > [ ]: optional, not present in C_EncryptFinal / C_DecryptFinal > > Co-authored-by: Francisco Ferrari <fferr...@redhat.com> > Co-authored-by: Martin Balao <mba...@redhat.com> The [PKCS #<wbr/>11 definition of `CKM_AES_CTS`](https://docs.oasis-open.org/pkcs11/pkcs11-curr/v3.0/pkcs11-curr-v3.0.html#_Toc30061253) refers to the [Addendum to NIST Special Publication 800-38A, "Recommendation for Block Cipher Modes of Operation: Three Variants of Ciphertext Stealing for CBC Mode"](https://doi.org/10.6028/NIST.SP.800-38A-Add) document. However, the standard does not prescribe which of the three CTS variants described by the document to use. This left the door open for PKCS #<wbr/>11 implementers to make different assumptions and create interoperability issues. We are aware that the _NSS Software Token_ implemented the CS1 variant (see [lib/freebl/cts.c](https://github.com/nss-dev/nss/blob/NSS_3_90_RTM/lib/freebl/cts.c#L64-L65)), but [public clarification had to be given in NSS Bug 373108](https://bugzilla.mozilla.org/show_bug.cgi?id=373108#c7). That's why we chose `CS1` as the `cipherTextStealingVariant` default. > As for the different variations, are you aware of different vendors > supporting different variations? At the moment of developing this enhancement, we were not aware of any other variant in use, but we had only evaluated NSS. The standard has a clear ambiguity, so there was a latent problem. Now, I have found another open source project using CS3 instead (see below). > This assumes prior knowledge on the variation used by underlying PKCS11 > library, right? Unfortunately yes, but it gives the user a chance to adjust for their PKCS11 library without the need of a _SunPKCS11_ update from our side. > Are all these variations under "CTS" name? Yes, they are known as "variants" —according to the mentioned NIST addendum— or "formats" —according to [Wikipedia](https://en.wikipedia.org/wiki/Ciphertext_stealing#Ciphertext_format "Wikipedia: Ciphertext stealing - Ciphertext format")—, always in the context of "Cipher Text Stealing". > Are they generally supported by all? According to _Wikipedia_, the most popular alternative is CS3. It also happens to be the Kerberos choice, and so the one implemented by _SunJCE_'s `AES/CTS/NoPadding`. However, NSS chose CS1 as it was the first one described in the NIST addendum, referred by the PKCS #<wbr/>11 standard. In the sake of interoperability, the PKCS #<wbr/>11 standard should choose one of the three variants. But since it is too late, PKCS #<wbr/>11 implementers supporting `CKM_AES_CTS` have already made their choice. ### OP-TEE provides a PKCS #<wbr/>11 interface, and has chosen CS3 Searching for other open source implementers, I have found that the [OP-TEE Trusted Execution Environment (TEE) exposes a PKCS #<wbr/>11 module](https://optee.readthedocs.io/en/latest/building/userland_integration.html). I don't know how widespread this project is, but it works as an example of how other implementors might behave. This PKCS #<wbr/>11 implementation uses CS3 for `CKM_AES_CTS`, contrary to NSS' choice of CS1: 1. The `CKM_AES_CTS` constant [has the `0x1089` value](https://github.com/openjdk/jdk/blob/jdk-23+24/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/PKCS11Constants.java#L889) 2. `CKM_AES_CTS` is [represented by the `PKCS11_CKM_AES_CTS` constant](https://github.com/OP-TEE/optee_os/blob/4.2.0/ta/pkcs11/include/pkcs11_ta.h#L1326) in OP-TEE 3. `PKCS11_CKM_AES_CTS` [is mapped to `TEE_ALG_AES_CTS`](https://github.com/OP-TEE/optee_os/blob/4.2.0/ta/pkcs11/src/processing_symm.c#L73) 4. When creating a `TEE_ALG_AES_CTS` context, [the `crypto_aes_cts_alloc_ctx()` function is used](https://github.com/OP-TEE/optee_os/blob/4.2.0/core/crypto/crypto.c#L136-L138) 5. This function [puts in the context a structure with operation callbacks that implement CTS](https://github.com/OP-TEE/optee_os/blob/4.2.0/core/crypto/aes-cts.c#L249) 6. The `cts_update.update` callback leads to `cts_update()`, which [then calls `cbc_cts_update()`](https://github.com/OP-TEE/optee_os/blob/4.2.0/core/crypto/aes-cts.c#L194-L195) 7. In `cbc_cts_update()`, [`len_last_block` is equal to `block_size` when the message would not require padding](https://github.com/OP-TEE/optee_os/blob/4.2.0/core/crypto/aes-cts.c#L113-L114) 8. Given the previous item fact, [the encryption code unconditionally swaps the last two blocks](https://github.com/OP-TEE/optee_os/blob/4.2.0/core/crypto/aes-cts.c#L130-L136), which corresponds to CS3 9. In addition, [CS3 is mentioned in a previous code comment](https://github.com/OP-TEE/optee_os/blob/4.2.0/core/crypto/aes-cts.c#L30) ------------- PR Comment: https://git.openjdk.org/jdk/pull/18898#issuecomment-2133588824
