8330842: Support AES CBC with Ciphertext Stealing (CTS) in SunPKCS11

[franferrax]

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.

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) requires buffering three blocks instead of two. If this bug gets fixed, the three block buffering 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 data source (padBuffer or input). This value is always a multiple of the block size. The relationship between the variables described so far is the following: totalInLen = dataForP11Update + newPadBufferLen.

The next step is to determine how many bytes from dataForP11Update must be passed to the token during the stage #​1 —the rest, if any, will be passed during the second stage—. For this calculation, we first determine how many bytes are needed to fill padBuffer up to a multiple of the block size and store this value in the variable fillLen. If plenty of bytes are available in the input for the token (dataForP11Update >= padBufferLen + fillLen), we complete padBuffer up to a multiple of the block size by taking bytes from the input and flush padBuffer entirely. Otherwise (dataForP11Update < padBufferLen + fillLen), we flush dataForP11Update bytes from padBuffer. Notice that in this case stage #​2 will be skipped, as there is nothing else available in the input for the token. Any remaining bytes in padBuffer are shifted to the beginning of the array. Stage #​3 may still buffer input to padBuffer.

Testing

Two tests were extended for AES CBC-CTS (TestSymmCiphersNoPad and TestSymmCiphers) and a new one introduced (TestCipherTextStealingMultipart). Test TestCipherTextStealingMultipart cross-checks encryption and decryption operations against SunJCE's implementation, splitting the input into multipart operations of different sizes. This test covers all execution paths in P11Cipher::implUpdate.

In addition, no regressions have been observed in the following test categories:

• jdk:tier1
• test/jdk/com/sun/crypto/provider/Cipher
• test/jdk/javax/crypto/Cipher
• test/jdk/sun/security/pkcs11/Cipher

This work is in collaboration with @martinuy.

Thanks.-

---

Progress

• [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue
• [ ] Change requires CSR request JDK-8330843 to be approved

Issues

• JDK-8330842: Support AES CBC with Ciphertext Stealing (CTS) in SunPKCS11 (Enhancement - P4)
• JDK-8330843: Support AES CBC with Ciphertext Stealing (CTS) in SunPKCS11 (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18898/head:pull/18898
$ git checkout pull/18898

Update a local copy of the PR:
$ git checkout pull/18898
$ git pull https://git.openjdk.org/jdk.git pull/18898/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18898

View PR using the GUI difftool:
$ git pr show -t 18898

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18898.diff

Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back fferrari! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk[bot]]

@franferrax This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8330842: Support AES CBC with Ciphertext Stealing (CTS) in SunPKCS11

Co-authored-by: Francisco Ferrari Bihurriet <[email protected]>
Co-authored-by: Martin Balao <[email protected]>
Reviewed-by: valeriep

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 274 new commits pushed to the master branch:

• 79a23017fc7154738c375fbb12a997525c3bf9e7: 8322859: Parallel: Move transform_stack_chunk
• 37e7698c29b8673b904945d397f0698ccd16d27b: 8335154: jcmd VM.classes -verbose=false does not set verbose to false
• f3b69da55a1ec4857fff1537a80ab1fefee93dac: 8335136: Underscore as parameter name in one-parameter functional types fails to compile
• 46b817b7499e74ba8812d38bcce93147ebf93b25: 8333363: ubsan: instanceKlass.cpp: runtime error: member call on null pointer of type 'struct AnnotationArray'
• 0fc5b2711fbdde972c40bfef2977dd9d70e09581: 8332014: since-checker - Fix @ since tags in jdk.jshell
• 9d20b58f40275002afa0348d94d5592a26894e88: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal
• 9bb675f89dd1eeec423ca96cb3f96d29f5de477c: 8334719: (se) Deferred close of SelectableChannel may result in a Selector doing the final close before concurrent I/O on channel has completed
• 6682305ee21cf595ec953d95bea594734a2982a8: 8334779: Test compiler/c1/CanonicalizeArrayLength.java is timing out
• 3796fdfcedc2b2202b72cca062218f840960414c: 8328536: javac - crash on unknown type referenced in yield statement
• 07bc523df85fde81bf736fedac62874d3cb11ee3: 8334670: SSLSocketOutputRecord buffer miscalculation
• ... and 264 more: https://git.openjdk.org/jdk/compare/cbb6747e6b9ce7e2b9e0ffb0a1f9499f7e0e13b0...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@valeriepeng) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk[bot]]

@franferrax The following label will be automatically applied to this pull request:

• security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge[bot]]

Webrevs

• 09: Full - Incremental (33a64b2c)
• 08: Full - Incremental (2c6a3c0f)
• 07: Full - Incremental (d6e96987)
• 06: Full - Incremental (a3151ef5)
• 05: Full - Incremental (476cbc97)
• 04: Full - Incremental (37d6eb80)
• 03: Full - Incremental (0d82e7e9)
• 02: Full - Incremental (997777e8)
• 01: Full - Incremental (68197517)
• 00: Full (938aaaec)

[valeriepeng]

I will take a look.

[valeriepeng]

As for the different variations, are you aware of different vendors supporting different variations? I quickly glanced through your changes, there is code adjusting data to different variations based on configuration setting. This assumes prior knowledge on the variation used by underlying PKCS11 library, right? Are all these variations under "CTS" name? Are they generally supported by all?

[franferrax]

The PKCS #11 definition of CKM_AES_CTS 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" 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 #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), but public clarification had to be given in NSS Bug 373108. 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—, 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 #11 standard.

In the sake of interoperability, the PKCS #11 standard should choose one of the three variants. But since it is too late, PKCS #11 implementers supporting CKM_AES_CTS have already made their choice.

OP-TEE provides a PKCS #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 #11 module. I don't know how widespread this project is, but it works as an example of how other implementors might behave.

This PKCS #11 implementation uses CS3 for CKM_AES_CTS, contrary to NSS' choice of CS1:

1. The CKM_AES_CTS constant has the 0x1089 value
2. CKM_AES_CTS is represented by the PKCS11_CKM_AES_CTS constant in OP-TEE
3. PKCS11_CKM_AES_CTS is mapped to TEE_ALG_AES_CTS
4. When creating a TEE_ALG_AES_CTS context, the crypto_aes_cts_alloc_ctx() function is used
5. This function puts in the context a structure with operation callbacks that implement CTS
6. The cts_update.update callback leads to cts_update(), which then calls cbc_cts_update()
7. In cbc_cts_update(), len_last_block is equal to block_size when the message would not require padding
8. Given the previous item fact, the encryption code unconditionally swaps the last two blocks, which corresponds to CS3
9. In addition, CS3 is mentioned in a previous code comment

[valeriepeng]

Well, I am somewhat concerned about the incompatibility between the CTS support in SunJCE provider (CS3) and the ambiguity of CTS support in various PKCS11 libraries. The default for PKCS11 is CS1 which is based on NSS but may not apply to other PKCS11 libraries. Also, if SunPKCS11 provider (using NSS) is placed before the SunJCE provider, wouldn't this lead to unexpected failure since they are using different variation? Instead of enabling CTS support by default, maybe we should only enable it when the configuration has the variation attribute set since users are required to know which CTS variation the underlying PKCS11 library implements...

[martinuy]

Well, I am somewhat concerned about the incompatibility between the CTS support in SunJCE provider (CS3) and the ambiguity of CTS support in various PKCS11 libraries. The default for PKCS11 is CS1 which is based on NSS but may not apply to other PKCS11 libraries. Also, if SunPKCS11 provider (using NSS) is placed before the SunJCE provider, wouldn't this lead to unexpected failure since they are using different variation? Instead of enabling CTS support by default, maybe we should only enable it when the configuration has the variation attribute set since users are required to know which CTS variation the underlying PKCS11 library implements...

We designed this feature with the idea that the SunPKCS11 provider has to translate to CS3 always and maintain compatibility with SunJCE. If the token configuration is correct, the translation will be right and you can interchange one provider with the other for the AES*/CTS/NoPadding algorithms. If the token configuration is not correct, the failure will be noticed, for example, when the SunPKCS11 provider is used for Kerberos CTS authentication. Other uses might be affected as well. We thought that defining a default value based on a widely used implementation of the PKCS 11 standard —used as a reference by other implementations— was reasonable and convenient for the user. There can be tokens implementing a different variation, it's impossible to rule out. I'm personally okay with not having a default value and forcing users to define the configuration property to enable CTS. Perhaps we can document in the CSR/guide that the NSS Software Token requires CS1 as an example. @franferrax what do you think?

[franferrax]

I'm personally okay with not having a default value and forcing users to define the configuration property to enable CTS. Perhaps we can document in the CSR/guide that the NSS Software Token requires CS1 as an example. @franferrax what do you think?

@martinuy: I agree, it makes sense to require the cipherTextStealingVariant configuration to be explicitly set in order for SunPKCS11 to register the Cipher services that depend on CKM_AES_CTS (AES*/CTS/NoPadding algorithm names).

Done in 997777e86c6fa03f070dcf0f219813c11cb480ce, I will adjust the CSR tomorrow.

New cipherTextStealingVariant behaviour

We slightly changed the error message format when an invalid value is passed. Here it is an example for cipherTextStealingVariant=CS4:

sun.security.pkcs11.ConfigurationException: cipherTextStealingVariant must be one of [CS1, CS2, CS3], read: Token[CS4], line 33

If no value is passed, the CKM_AES_CTS mechanism is handled as if it were disabled by the configuration. This is the message logged when running with -Djava.security.debug=sunpkcs11:

Mechanism CKM_AES_CTS:
  ulMinKeySize: 16
  ulMaxKeySize: 32
  flags: 768 = CKF_ENCRYPT | CKF_DECRYPT
DISABLED due to an unspecified cipherTextStealingVariant in configuration

As an exception, to improve the user experience, when no value is passed but the token is NSS (by means of the token label), cipherTextStealingVariant is assumed to be CS1.

[franferrax]

Done in 997777e86c6fa03f070dcf0f219813c11cb480ce, I will adjust the CSR tomorrow.

JDK-8330843 CSR updated accordingly.

[valeriepeng]

Well, given the time frame, maybe we should re-target the CSR to jdk 24?

[valeriepeng]

Done in 997777e, I will adjust the CSR tomorrow.

JDK-8330843 CSR updated accordingly.

The CSR is quite lengthy and some wordings may be more suitable for another section, so I made some adjustments. In addition, I made some minor wording changes to the specification section to be more align with the existing guide. Could you please take a second look? I will add myself as the reviewer once we settle on the changes.

[franferrax]

The CSR is quite lengthy and some wordings may be more suitable for another section, so I made some adjustments. In addition, I made some minor wording changes to the specification section to be more align with the existing guide. Could you please take a second look? I will add myself as the reviewer once we settle on the changes.

I gave it a look and made some minor updates.

[valeriepeng]

I created the release-note and doc sub-task for this RFE. Please take a look. As for the code change, the rest looks fine to me. Thanks, Valerie

[franferrax]

I created the release-note and doc sub-task for this RFE. Please take a look.

They look good to me, I just removed two extra white spaces around the closing parenthesis in JDK-8333760.

As for the code change, the rest looks fine to me.

I did one more minor change: 2c6a3c0f79809db77b28c21244ced6621903039f.

[seanjmullan]

In the release note, it says "To ensure interoperability with SunJCE and Kerberos which uses the CS3 variant (see https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38a-add.pdf, a new SunPKCS11 provider configuration attribute named "cipherTextStealingVariant" is introduced and must be set with any of the following values: CS1, CS2 or CS3 to indicate the CTS variant of the underlying PKCS11 library except for NSS as it's known to be CS1."

I didn't understand the interoperability part. If SunJCE and Kerberos use CS3, then how can PKCS11 ensure interoperability if someone sets the variable to CS1 or CS2?

Also, if the property is set to CS2 or CS3, and you are using NSS, is an exception or error thrown?

[martinuy]

@franferrax Can you please quote the relevant fragment from the original CSR text? I think it was more clear.

[franferrax]

@martinuy

@franferrax Can you please quote the relevant fragment from the original CSR text? I think it was more clear.

This was the original CSR text that corresponds with the part of the CSR copied in the release note:

Introduce a new SunPKCS11 provider configuration attribute named cipherTextStealingVariant that must be set with any of the following values: CS1, CS2 or CS3. This attribute can be used to specify the token's CTS variant and is required to enable CKM_AES_CTS. The AES CBC-CTS transformations are not registered by SunPKCS11 if the cipherTextStealingVariant attribute is not present, with an exception for the NSS Software Token where CS1 is assumed by default. After encryption, the ciphertext will be converted from the token's variant to CS3. Before decryption, the ciphertext will be converted from CS3 to the token's variant.

@seanjmullan

I didn't understand the interoperability part. If SunJCE and Kerberos use CS3, then how can PKCS11 ensure interoperability if someone sets the variable to CS1 or CS2?

The interoperability is ensured by internally converting between CS3 and the PKCS #​11 library variant, so that ciphertexts are always arranged in the CS3 variant, from a public APIs user's perspective.

Also, if the property is set to CS2 or CS3, and you are using NSS, is an exception or error thrown?

No, an exception is not thrown and the chosen CS2 or CS3 variant is applied even for NSS. NOTE: this misconfiguration will lead to invalid outputs. This behaviour is the same for any PKCS #​11 library. What we provide for NSS is an overridable default.

[franferrax]

/contributor add fferrari

[franferrax]

/contributor add mbalao

[openjdk[bot]]

@franferrax Contributor Francisco Ferrari Bihurriet <[email protected]> successfully added.

[openjdk[bot]]

@franferrax Contributor Martin Balao <[email protected]> successfully added.

[franferrax]

/integrate

[openjdk[bot]]

@franferrax Your change (at version 33a64b2c59a603343032696f632dd052f37ba4f9) is now ready to be sponsored by a Committer.

[martinuy]

/sponsor

[openjdk[bot]]

Going to push as commit 4ab7e98c79a1a0b7aba1ca74a8316820c906e70e. Since your change was applied there have been 278 commits pushed to the master branch:

• 5909d54147355dd7da5786ff39ead4c15816705c: 8326820: Metadata artificially kept alive
• d5375c7db658de491c1f5bad053040d21b82941e: 8333308: javap --system handling doesn't work on internal class names
• 6b961acb87c29027f2158c6b7a764f1276a0bf52: 8333786: Serial: Remove SerialHeap::_incremental_collection_failed
• 50dd962b0d0fe36634d96dbbd9d94fbc34d9ff7f: 8335007: Inline OopMapCache table
• 79a23017fc7154738c375fbb12a997525c3bf9e7: 8322859: Parallel: Move transform_stack_chunk
• 37e7698c29b8673b904945d397f0698ccd16d27b: 8335154: jcmd VM.classes -verbose=false does not set verbose to false
• f3b69da55a1ec4857fff1537a80ab1fefee93dac: 8335136: Underscore as parameter name in one-parameter functional types fails to compile
• 46b817b7499e74ba8812d38bcce93147ebf93b25: 8333363: ubsan: instanceKlass.cpp: runtime error: member call on null pointer of type 'struct AnnotationArray'
• 0fc5b2711fbdde972c40bfef2977dd9d70e09581: 8332014: since-checker - Fix @ since tags in jdk.jshell
• 9d20b58f40275002afa0348d94d5592a26894e88: 8334328: Reduce object allocation for FloatToDecimal and DoubleToDecimal
• ... and 268 more: https://git.openjdk.org/jdk/compare/cbb6747e6b9ce7e2b9e0ffb0a1f9499f7e0e13b0...master

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@martinuy @franferrax Pushed as commit 4ab7e98c79a1a0b7aba1ca74a8316820c906e70e.

:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.