On Wed, 5 Jun 2024 19:06:14 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 > 13 additional commits since the last revision: > > - Fix penultimate block length calculation > > It is not correct to calculate the penultimate block length based on > the output array offset, since the output array can include arbitrary > user-supplied data. > > Add a test case to check this fix. > > Co-authored-by: Francisco Ferrari <fferr...@redhat.com> > Co-authored-by: Martin Balao <mba...@redhat.com> > - Extract swapLastTwoBlocks() unified logic > > Co-authored-by: Francisco Ferrari <fferr...@redhat.com> > Co-authored-by: Martin Balao <mba...@redhat.com> > - Merge 'openjdk/master' into JDK-8330843 > - Apply code-review suggestion > > Co-authored-by: Francisco Ferrari <fferr...@redhat.com> > Co-authored-by: Martin Balao <mba...@redhat.com> > - Improve handling when the token variant is unknown > > Avoid registering CTS algorithms (those depending on CKM_AES_CTS) when > the token CTS variant has not been specified in the configuration. Make > NSS an exception, as we know that it uses the CS1 variant. > > Take advantage to extract a pkcs11.Config::parseEnumEntry() method for > a cleaner entry in the main switch statement of pkcs11.Config::parse(), > also slightly improving the error message. > > Co-authored-by: Francisco Ferrari <fferr...@redhat.com> > Co-authored-by: Martin Balao <mba...@redhat.com> > - Merge 'openjdk/master' into JDK-8330843 > - 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> > C... src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Token.java line 70: > 68: > 69: @SuppressWarnings("serial") // Type of field is not Serializable > 70: final CTSVariant ctsVariant; For new non-serializable fields, you should just mark it with "transient" keyword and no need for the `@SuppressWarnings("serial")` ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18898#discussion_r1628342501
