8326609: New AES implementation with updates specified in FIPS 197

[smemery]

General:

i) This work is to replace the existing AES cipher under the Cryptix license.

ii) The lookup tables are employed for performance, but also for operating in constant time.

iii) Several loops have been unrolled for optimization purposes, but are harder to read and don't meet coding style guidelines.

iv) None of the AES related intrinsics has been modified in this PR, but the new code has been updated to use the intrinsics related hooks for the AES block and key table arguments.

Note: I've purposefully not seen the original Cryptix code, so when making a code review comment please don't quote the code in the AESCrypt.java file.

Correctness:

The following AES-specific regression tests have passed in intrinsics (default) and non-intrinsic (-Xint) modes:

i) test/jdk/com/sun/crypto/provider/Cipher/AES: all 27 tests pass

-intrinsics mode for:

ii) test/hotspot/jtreg/compiler/codegen/aes: all 4 tests pass

iii) jck:api/java_security, jck:api/javax_crypto, jck:api/javax_net, jck:api/javax_security, jck:api/org_ietf, and jck:api/javax_xml/crypto: passed, with 10 known failures

iv) jdk_security_infra: passed, with 48 known failures

v) tier1 and tier2: all 110257 tests pass

Security:

In order to prevent side-channel (timing and differential power analysis) attacks the code has been constructed to operate in constant time and does not use conditionals based on the key or key expansion table. This is accomplished by using lookup tables in both the cipher and inverse cipher of AES.

Performance:

All AES related benchmarks have been executed against the new and original Cryptix code:

micro:org.openjdk.bench.javax.crypto.AES

micro:org.openjdk.bench.javax.crypto.full.AESBench

micro:org.openjdk.bench.javax.crypto.full.AESExtraBench

micro:org.openjdk.bench.javax.crypto.full.AESGCMBench

micro:org.openjdk.bench.javax.crypto.full.AESGCMByteBuffer

micro:org.openjdk.bench.javax.crypto.full.AESGCMCipherInputStream

micro:org.openjdk.bench.javax.crypto.full.AESGCMCipherOutputStream

micro:org.openjdk.bench.javax.crypto.full.AESKeyWrapBench.

micro:org.openjdk.bench.java.security.CipherSuiteBench (AES)

The benchmarks were executed in different compiler modes (default (no compiler options), -Xint, and -Xcomp) and on two different architectures (x86 and arm64) with the following encryption results:

i) Default (no JVM options, non-intrinsics) mode:

a) Encryption: the new code performed better for both architectures tested (x86: +7.6%, arm64: +3.5%)

Analysis: the new code makes some optimizations in the last cipher round with the T2 lookup table that Cryptix may not, hence the better performance in non-intrinsics.

b) Decryption: the new code performed mixed between architectures tested (x86: +8.3%, arm64: -1.1%)

Analysis: the new code performs predominately better, except for decryption on arm64, which we believe is negligible and acceptable to have noticeably better performance with x86 decryption.

ii) Default (no JVM options, intrinsics) mode:

a) Encryption and Decryption: as expected, both the new code and Cryptix code performed similarly (within the error margins)

Analysis: this is from the fact that the intrinsics related code has not changed with the new changes.

iii) Interpreted-only (-Xint) mode:

a) Encryption: the new code performed better than the Cryptix code for both architectures (x86: +0.6%, arm64: +6.0%).

b) Decryption: the new code performed slightly worse than the Cryptix code for both architectures (x86: -3.3%, arm64: -2.4%).

Analysis: the design of the new code was focused on instruction efficiency; eliminating unnecessary index variables, rolling out the rounds loop, and using no objects for round and inverse round transforms. This is especially noticeable in arm64 encryption, but we believe that decryption's slight drop in performance is negligible.

iv) JIT compiler (-Xcomp) mode:

a) Encryption: in this mode, performance is mixed performant between the two architectures tested (x86: +11.7%, arm64: +1.5%).

b) Decryption: performance is decreases for both of the architectures tested (x86: -4.9%, arm64: -3.2%).

Analysis: As with the no options results, we believe that the increase in performance for both architectures in encryption is most likely from the T2 gadgetry that we've implemented. We believe that the slight performance drop in decryption is negligible. In any case, the -Xcomp option is primarily used for debugging purposes, ergo we are not as concerned about this slight drop.

Resource utilization:

The new AES code uses similar resources to that of the existing Cryptix code. Memory allocation has the following characteristics:

i) Peak allocation for both Cryptix and the new code is only a fraction of a percentage point different for both the 1 cipher object and 10 cipher objects test.

Analysis: We believe that this is negligible given the difference in the 20ms to 50ms window of peak allocation.

ii) Total GC pause for Cryptix and the new code only differs by less than 5% for both the 1 object and 10 objects test.

Analysis: This is acceptable behavior given that the benchmark performance for the new code is better overall.

iii) Peak pre-GC allocation for Cryptix and the new code is only a fraction of a percent more for the new code in the 1 object case and is only 2% more for the 10 objects case.

Analysis: These differences indicate ~500 bytes per object discrepancy between the Cryptix and new code, which is also negligible.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8326609: New AES implementation with updates specified in FIPS 197 (Enhancement - P3)

Reviewers

• Valerie Peng (@valeriepeng - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27807

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back smemery! 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]

@smemery 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:

8326609: New AES implementation with updates specified in FIPS 197

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 162 new commits pushed to the master branch:

• 3e20a93: 8370156: Fix jpackage IconTest
• be18e7e: 8064922: [macos] Test javax/swing/JTabbedPane/4624207/bug4624207.java fails
• 0744db8: 8367002: Missing compiled exception handler for "recursive" exception
• ... and 159 more: https://git.openjdk.org/jdk/compare/4ca4485e9af10a49ca95710c4e26aa3895835d47...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]

@smemery The following labels will be automatically applied to this pull request:

• core-libs
• graal
• hotspot-compiler
• security

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

[mlbridge]

Webrevs

• 12: Full - Incremental (fdfd3892)
• 11: Full - Incremental (5ea6933b)
• 10: Full - Incremental (e0741b17)
• 09: Full - Incremental (1102c609)
• 08: Full - Incremental (2ce35b97)
• 07: Full - Incremental (a5991a2f)
• 06: Full - Incremental (9f00c355)
• 05: Full - Incremental (fbf2117f)
• 04: Full - Incremental (f48160cf)
• 03: Full - Incremental (3fc25aef)
• 02: Full - Incremental (07003719)
• 01: Full - Incremental (af0f9c4c)
• 00: Full (1f742c1f)

[openjdk]

@smemery hotspot has been added to this pull request based on files touched in new commit(s).

[valeriepeng]

Just have some minor questions and nit.

[valeriepeng]

Changes look fine, thanks~

[smemery]

Thank you @valeriepeng, @iwanowww, and @eme64 for your comments. After code review comment updates, all recommended regression tests have been executed and have passed (with all known failures). Benchmarks have been reran in each of the modes supported by intrinsics and these results have matched to those of pre-code review update.

/integrate

[openjdk]

@smemery
Your change (at version fdfd389) is now ready to be sponsored by a Committer.

[valeriepeng]

/sponsor

[openjdk]

Going to push as commit 62f11cd.
Since your change was applied there have been 176 commits pushed to the master branch:

• b2e431a: 8369068: GenShen: Generations still aren't reconciled assertion failure
• b0721e2: 8368982: Test sun/security/tools/jarsigner/EC.java completed and timed out
• a0e0b2d: 8370057: Correct scale handling of BigDecimal.sqrt
• ... and 173 more: https://git.openjdk.org/jdk/compare/4ca4485e9af10a49ca95710c4e26aa3895835d47...master

Your commit was automatically rebased without conflicts.

[openjdk]

@valeriepeng @smemery Pushed as commit 62f11cd.

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