Modernize implementation #85
Conversation
by internalizing CMAC implementation
to avoid byte[] allocations in `com.sun.crypto.provider.CipherCore.doFinal(byte[], int, int, byte[], int)`
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
# Conflicts: # pom.xml
[deploy]
[ci skip]
[ci skip]
[deploy]
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes the SIV-mode implementation by removing all external dependencies (BouncyCastle, Jetbrains Annotations), internalizing the CMAC implementation, and introducing a new API while maintaining backward compatibility. The changes achieve a 60% reduction in allocations resulting in 15-25% performance improvement.
Key Changes:
- Introduced new
SivEngineAPI for direct encryption/decryption operations - Implemented JCE Security Provider (
SivProvider) withCipherandMacSPIs - Internalized CMAC implementation to eliminate BouncyCastle dependency
- Deprecated legacy
SivModeAPI while maintaining backward compatibility
Reviewed Changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/cryptomator/siv/SivEngine.java | New core encryption engine with modern API |
| src/main/java/org/cryptomator/siv/SivCipher.java | JCE Cipher SPI implementation for standard JCA usage |
| src/main/java/org/cryptomator/siv/SivProvider.java | Security Provider registration for CMAC and AES/SIV/NoPadding |
| src/main/java/org/cryptomator/siv/CMac.java | Internalized CMAC implementation following RFC 4493 |
| src/main/java/org/cryptomator/siv/Utils.java | Utility methods extracted for cryptographic operations |
| src/main/java/org/cryptomator/siv/SivMode.java | Deprecated legacy API, now delegates to SivEngine |
| src/test/java/org/cryptomator/siv/SivEngineTest.java | Comprehensive test coverage for new SivEngine API |
| pom.xml | Removed BouncyCastle dependency and maven-shade-plugin |
| README.md | Updated usage examples to reflect new API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR refactors the AES-SIV implementation to remove external dependencies on BouncyCastle and Jetbrains Annotations. It introduces custom implementations for CMAC (RFC 4493) via a new CMac class and reimplements the SIV logic in a new SivEngine class. The legacy SivMode API is deprecated and redirected to SivEngine. New JCE integration is provided through SivCipher (CipherSpi) and SivProvider for standard Java security APIs. Internal utilities (ThreadLocals, placeholder shim classes) and older CTR computer implementations are removed. The Maven POM is updated to version 2.0.0-SNAPSHOT with build configuration changes including removal of maven-shade-plugin. Documentation and tests are updated to reflect the new APIs. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (15)
README.md (1)
11-17: Align README usage with the new primary APIs and actual signaturesThe feature bullet (“No dependencies”) and version
2.0.0look consistent with the refactor, but the usage section still only showsSivModeand anew SivMode(key)API. Given thatSivModeis being deprecated in favor ofSivEngineand JCE usage viaSivProvider, consider:
- Explicitly documenting
SivEngine-based usage and the JCE provider flow as the preferred APIs.- Clearly marking the
SivModeexample as deprecated/legacy, or replacing it entirely if the public surface has changed.- Double-checking that the constructor and encrypt/decrypt overloads in the snippet match the actual
SivMode(orSivEngine) signatures to avoid confusing downstream users.Also applies to: 27-39, 49-50
src/main/java/org/cryptomator/siv/UnauthenticCiphertextException.java (1)
8-9: Complement the annotation with a@deprecatedJavadoc hintMarking
UnauthenticCiphertextExceptionas@Deprecatedis consistent with the new API direction. Consider also adding a@deprecatedJavadoc tag that briefly points users to the modern error path (e.g., viaSivEngineandAEADBadTagException) to make the migration story explicit in generated Javadocs.src/test/java/org/cryptomator/siv/CMacTest.java (1)
16-35: CMAC vector coverage looks solid; HexConverter could be a bit more defensiveThe parameterized tests against RFC 4493 and NIST vectors are well-structured and should give strong coverage of the new
CMac.tagimplementation. As an optional improvement for future maintenance:
- In
HexConverter.convert, consider validating that the hex string has even length and contains only valid hex characters, and throw anArgumentConversionExceptionotherwise. That makes failures clearer if a vector line is ever edited incorrectly.Not required for this PR, but it would make the test utility more robust to accidental input errors.
Also applies to: 49-85
src/main/java/org/cryptomator/siv/Utils.java (1)
10-13: Consider keepingUtilsnon-public and documenting in-place semanticsSince all methods in
Utilsare package-private and used internally by SIV/CMAC code, exposing the class itself aspublicslightly widens the library’s API surface without a strong need.As an optional cleanup:
- Make the class package-private (
final class Utils) so it’s clearly an internal helper.- Optionally add brief comments on
dblandxornoting that they operate in-place on their first argument (and, fordbl, on the provided array), to prevent accidental misuse by future contributors.No functional issues, but this helps keep the public API minimal and the in-place behavior explicit.
Also applies to: 34-57
src/main/java/org/cryptomator/siv/SivProvider.java (1)
15-29: Provider wiring for CMAC and AES/SIV looks good; consider minor hardeningThe singleton provider and
putServiceregistrations are consistent with the new CMAC andSivCipherimplementations. To make the API surface a bit tighter you could:
- Declare
SivProviderasfinalto avoid subclassing of a security-sensitive type.- Explicitly commit in Javadoc to the algorithm strings
"CMAC"and"AES/SIV/NoPadding"as the long‑term JCE names, since changing them later would be a breaking change.Please double‑check against your existing public documentation/usages that these transformation names are exactly what you want to support going forward.
src/test/java/org/cryptomator/siv/SivProviderTest.java (1)
18-50: Good coverage of provider integration; optionally isolate global Security stateThe tests exercise CMAC and AES/SIV discovery both via explicit provider instance, provider name, and default lookup, plus a simple encrypt/decrypt flow—solid coverage. The only refinement I’d suggest is to centralize
Security.addProvider(SivProvider.INSTANCE)in a@BeforeAlland restore the original provider list in an@AfterAll(or useinsertProviderAt+removeProvider) to avoid leaking provider registration across unrelated tests.Please ensure no other tests depend on a specific provider ordering before/after these run.
src/main/java/org/cryptomator/siv/CMac.java (1)
95-135: Optional: reuse the last-block buffer to reduce per-call allocations
engineDoFinalallocates a freshbyte[BLOCK_SIZE]form_laston every MAC computation. Given CMAC may be invoked in tight loops and this PR targets allocation reductions, you could reuse a dedicated field for that buffer:public class CMac extends MacSpi { @@ - private final byte[] buffer = new byte[BLOCK_SIZE]; + private final byte[] buffer = new byte[BLOCK_SIZE]; + private final byte[] mLast = new byte[BLOCK_SIZE]; @@ @Override protected byte[] engineDoFinal() { @@ - byte[] m_last = new byte[BLOCK_SIZE]; + Arrays.fill(mLast, (byte) 0); if (flag) { // M_last := M_n XOR K1; - xor(buffer, k1, m_last); + xor(buffer, k1, mLast); } else { @@ - xor(buffer, k2, m_last); + xor(buffer, k2, mLast); } @@ - xor(x, m_last, y); // Y := M_last XOR X; + xor(x, mLast, y); // Y := M_last XOR X; @@ @Override protected void engineReset() { @@ Arrays.fill(buffer, (byte) 0); Arrays.fill(x, (byte) 0); Arrays.fill(y, (byte) 0); + Arrays.fill(mLast, (byte) 0); }You still need to allocate the output tag array for each call (to avoid sharing internal buffers with the caller), but this change removes one transient 16‑byte allocation per MAC.
Please confirm this micro‑optimization is actually meaningful in your benchmarks before adopting it; the change is purely optional.
src/main/java/org/cryptomator/siv/SivCipher.java (1)
110-121: Optional: reduce copying in engineUpdate for large inputs
engineUpdatecurrently growsinputBufferviaArrays.copyOfon every call, which is O(n²) if callers feed data in many small chunks. Since SIV inherently needs the full plaintext in memory, you might still want to avoid quadratic copying by accumulating into a growable buffer:
- Use a
ByteArrayOutputStream(with an initial capacity hint) instead of a rawbyte[], converting to a byte array only inengineDoFinal, or- Maintain your own
(byte[] buffer, int len)pair, expanding geometrically (e.g., 2×) when capacity is exceeded.This is not correctness‑critical but could matter for workloads that stream input in smaller pieces.
If you have benchmarks with many small
updatecalls, it’s worth checking whether this change delivers measurable gains before implementing it.src/test/java/org/cryptomator/siv/UtilsTest.java (1)
56-63: Remove duplicated XOR assertion and exercise the 3‑argument overloadLines 60 and 61 perform the exact same assertion, which is redundant and misses the chance to test
xor(in1, in2, out).You could refactor like this to keep the semantics but cover the 3‑arg variant:
@@ - Assertions.assertArrayEquals(new byte[]{(byte) 0x01, (byte) 0x02, (byte) 0x03}, xor(new byte[]{(byte) 0xFF, (byte) 0x55, (byte) 0x81}, new byte[]{(byte) 0xFE, (byte) 0x57, (byte) 0x82})); - Assertions.assertArrayEquals(new byte[]{(byte) 0x01, (byte) 0x02, (byte) 0x03}, xor(new byte[]{(byte) 0xFF, (byte) 0x55, (byte) 0x81}, new byte[]{(byte) 0xFE, (byte) 0x57, (byte) 0x82})); + Assertions.assertArrayEquals(new byte[]{(byte) 0x01, (byte) 0x02, (byte) 0x03}, xor(new byte[]{(byte) 0xFF, (byte) 0x55, (byte) 0x81}, new byte[]{(byte) 0xFE, (byte) 0x57, (byte) 0x82})); + byte[] out = new byte[3]; + Assertions.assertArrayEquals( + new byte[]{(byte) 0x01, (byte) 0x02, (byte) 0x03}, + xor(new byte[]{(byte) 0xFF, (byte) 0x55, (byte) 0x81}, new byte[]{(byte) 0xFE, (byte) 0x57, (byte) 0x82}, out));src/test/java/org/cryptomator/siv/SivCipherTest.java (1)
108-134: Wrap/unwrap test gives good streaming and AAD coverage; consider adding a tampering caseThe wrap/unwrap test nicely covers:
- AAD via both
ByteBufferand(byte[], off, len)forms.- Streaming
engineUpdatecalls that produce no output untilengineDoFinal.- Full round‑trip correctness across WRAP/UNWRAP modes.
As a follow‑up, you might add a second test that flips one byte in
wrappedor changes AAD and assertsAEADBadTagExceptionfromengineDoFinal, to lock in the authentication failure behaviour as well.src/main/java/org/cryptomator/siv/SivEngine.java (2)
46-57: Optionally zeroize intermediate key material after creatingSecretKeySpecsThe constructor copies the user key into two temporary arrays (
macKey,ctrKey) and then passes them intoSecretKeySpec, which itself copies the bytes. At that point the local arrays are no longer needed but still hold key material until GC.You could clear them to slightly reduce key lifetime in process memory:
final byte[] macKey = new byte[subkeyLen]; final byte[] ctrKey = new byte[subkeyLen]; System.arraycopy(key, 0, macKey, 0, macKey.length); System.arraycopy(key, macKey.length, ctrKey, 0, ctrKey.length); this.macKey = new SecretKeySpec(macKey, "AES"); this.ctrKey = new SecretKeySpec(ctrKey, "AES"); + + // Best‑effort cleanup of intermediate key material + Arrays.fill(macKey, (byte) 0); + Arrays.fill(ctrKey, (byte) 0);Not mandatory, but a cheap hardening step given this is cryptographic code.
83-92: Consider mirroring the overflow guard in the convenienceencryptoverloadThe streaming
encryptoverload defends againstplaintext.length > Integer.MAX_VALUE - IV_LENGTH, but theencrypt(byte[] plaintext, byte[]... associatedData)variant allocatesnew byte[IV_LENGTH + plaintext.length]without that check. For extremely large inputs this could overflow the length computation before the allocation fails cleanly.You could early‑reject the same way here:
public byte[] encrypt(byte[] plaintext, byte[]... associatedData) { + if (plaintext.length > (Integer.MAX_VALUE - IV_LENGTH)) { + throw new IllegalArgumentException("Plaintext is too long"); + } final byte[] ciphertext = new byte[IV_LENGTH + plaintext.length];src/main/java/org/cryptomator/siv/SivMode.java (2)
69-90: Dual-byte[] encrypt: key concatenation is correct; consider explicit null validationThe core
encrypt(byte[] ctrKey, byte[] macKey, ...):
- Concatenates
macKeythenctrKeyintocombinedKey, matchingEncryptionTestCase.getKey()and RFC-style MAC||ENC ordering.- Delegates to
new SivEngine(combinedKey).encrypt(...)and wipescombinedKeyinfinally.One thing you might tighten: currently passing
nullfor either key will throwNullPointerExceptionwhen accessing.length, whereas the Javadoc promisesIllegalArgumentExceptionfor invalid keys. If you care about strict API behavior (even though this is deprecated), add explicit null checks and throwIllegalArgumentExceptionto match the contract.- public byte[] encrypt(byte[] ctrKey, byte[] macKey, byte[] plaintext, byte[]... associatedData) { - byte[] combinedKey = new byte[ctrKey.length + macKey.length]; + public byte[] encrypt(byte[] ctrKey, byte[] macKey, byte[] plaintext, byte[]... associatedData) { + if (ctrKey == null || macKey == null) { + throw new IllegalArgumentException("Keys must not be null."); + } + byte[] combinedKey = new byte[ctrKey.length + macKey.length];
146-171: Dual-byte[] decrypt: ordering and AEAD mapping look correct; same optional null-guard appliesThe core
decrypt(byte[] ctrKey, byte[] macKey, ...):
- Builds
combinedKeyasmacKey || ctrKey, aligned with encryption andEncryptionTestCase.getKey().- Delegates to
SivEngine.decrypt, translatingAEADBadTagExceptionintoUnauthenticCiphertextException.- Always wipes
combinedKeyinfinally, even on failure.As with the encrypt counterpart, you may want to explicitly guard against
nullkeys and throwIllegalArgumentExceptioninstead of an implicitNullPointerExceptionto better match the Javadoc on invalid keys.- public byte[] decrypt(byte[] ctrKey, byte[] macKey, byte[] ciphertext, byte[]... associatedData) + public byte[] decrypt(byte[] ctrKey, byte[] macKey, byte[] ciphertext, byte[]... associatedData) throws UnauthenticCiphertextException, IllegalBlockSizeException { - byte[] combinedKey = new byte[ctrKey.length + macKey.length]; + if (ctrKey == null || macKey == null) { + throw new IllegalArgumentException("Keys must not be null."); + } + byte[] combinedKey = new byte[ctrKey.length + macKey.length];src/test/java/org/cryptomator/siv/SivEngineTest.java (1)
315-329: Generated testcases loader fixes prior resource leak; consider handling missing resource explicitlyThe
GeneratedTestCases.loadTestCases()method now uses a try-with-resources block that closesInputStream,Reader, andBufferedReader, addressing the earlier static-analysis warning about leakingInputStreamReader.One optional enhancement: if
/testcases.txtis missing,getResourceAsStreamwill returnnullandnew InputStreamReader(in, ...)will throw aNullPointerException. If you want clearer diagnostics, you could either:
- Assert with a descriptive failure, or
- Use
Assumptions.assumeTrue(in != null, "...")to skip the generated-vector tests when the resource is unavailable.This is non-blocking but would make failures easier to interpret.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
.github/workflows/build.yml(4 hunks).github/workflows/codeql-analysis.yml(1 hunks).github/workflows/publish-central.yml(0 hunks).github/workflows/publish-github.yml(0 hunks)CHANGELOG.md(1 hunks)README.md(3 hunks)pom.xml(6 hunks)src/main/java/org/cryptomator/siv/CMac.java(1 hunks)src/main/java/org/cryptomator/siv/CustomCtrComputer.java(0 hunks)src/main/java/org/cryptomator/siv/JceAesBlockCipher.java(0 hunks)src/main/java/org/cryptomator/siv/JceAesCtrComputer.java(0 hunks)src/main/java/org/cryptomator/siv/SivCipher.java(1 hunks)src/main/java/org/cryptomator/siv/SivEngine.java(1 hunks)src/main/java/org/cryptomator/siv/SivMode.java(7 hunks)src/main/java/org/cryptomator/siv/SivProvider.java(1 hunks)src/main/java/org/cryptomator/siv/ThreadLocals.java(0 hunks)src/main/java/org/cryptomator/siv/UnauthenticCiphertextException.java(2 hunks)src/main/java/org/cryptomator/siv/Utils.java(1 hunks)src/main/java/org/cryptomator/siv/org/bouncycastle/crypto/Placeholder.java(0 hunks)src/main/java/org/cryptomator/siv/org/bouncycastle/crypto/macs/Placeholder.java(0 hunks)src/main/java/org/cryptomator/siv/org/bouncycastle/crypto/modes/Placeholder.java(0 hunks)src/main/java/org/cryptomator/siv/org/bouncycastle/crypto/paddings/Placeholder.java(0 hunks)src/main/java/org/cryptomator/siv/org/bouncycastle/crypto/params/Placeholder.java(0 hunks)src/main/java/org/cryptomator/siv/org/bouncycastle/util/Placeholder.java(0 hunks)src/main/java/org/cryptomator/siv/package-info.java(1 hunks)src/main/java9/module-info.java(1 hunks)src/main/java9/org.cryptomator.siv/ThreadLocals.java(0 hunks)src/main/resources/META-INF/services/java.security.Provider(1 hunks)src/test/java/org/cryptomator/siv/BenchmarkTest.java(0 hunks)src/test/java/org/cryptomator/siv/CMacTest.java(1 hunks)src/test/java/org/cryptomator/siv/CustomCtrComputerTest.java(0 hunks)src/test/java/org/cryptomator/siv/EncryptionTestCase.java(2 hunks)src/test/java/org/cryptomator/siv/JceAesBlockCipherTest.java(0 hunks)src/test/java/org/cryptomator/siv/JceAesCtrComputerTest.java(0 hunks)src/test/java/org/cryptomator/siv/SivCipherTest.java(1 hunks)src/test/java/org/cryptomator/siv/SivEngineTest.java(1 hunks)src/test/java/org/cryptomator/siv/SivModeBenchmark.java(2 hunks)src/test/java/org/cryptomator/siv/SivModeTest.java(1 hunks)src/test/java/org/cryptomator/siv/SivProviderTest.java(1 hunks)src/test/java/org/cryptomator/siv/UtilsTest.java(1 hunks)
💤 Files with no reviewable changes (17)
- src/test/java/org/cryptomator/siv/BenchmarkTest.java
- .github/workflows/publish-central.yml
- src/main/java9/org.cryptomator.siv/ThreadLocals.java
- src/main/java/org/cryptomator/siv/org/bouncycastle/crypto/paddings/Placeholder.java
- src/main/java/org/cryptomator/siv/org/bouncycastle/crypto/Placeholder.java
- src/main/java/org/cryptomator/siv/org/bouncycastle/crypto/modes/Placeholder.java
- src/main/java/org/cryptomator/siv/org/bouncycastle/crypto/macs/Placeholder.java
- src/test/java/org/cryptomator/siv/JceAesBlockCipherTest.java
- src/main/java/org/cryptomator/siv/JceAesCtrComputer.java
- src/main/java/org/cryptomator/siv/ThreadLocals.java
- src/test/java/org/cryptomator/siv/CustomCtrComputerTest.java
- src/main/java/org/cryptomator/siv/org/bouncycastle/crypto/params/Placeholder.java
- src/main/java/org/cryptomator/siv/CustomCtrComputer.java
- src/main/java/org/cryptomator/siv/org/bouncycastle/util/Placeholder.java
- src/test/java/org/cryptomator/siv/JceAesCtrComputerTest.java
- .github/workflows/publish-github.yml
- src/main/java/org/cryptomator/siv/JceAesBlockCipher.java
🧰 Additional context used
🧬 Code graph analysis (10)
src/test/java/org/cryptomator/siv/UtilsTest.java (1)
src/main/java/org/cryptomator/siv/Utils.java (1)
Utils(10-59)
src/test/java/org/cryptomator/siv/SivProviderTest.java (1)
src/main/java/org/cryptomator/siv/SivProvider.java (1)
SivProvider(15-31)
src/test/java/org/cryptomator/siv/SivModeTest.java (2)
src/main/java/org/cryptomator/siv/SivMode.java (1)
Deprecated(13-173)src/main/java/org/cryptomator/siv/UnauthenticCiphertextException.java (1)
Deprecated(8-21)
src/main/java/org/cryptomator/siv/CMac.java (1)
src/main/java/org/cryptomator/siv/Utils.java (1)
Utils(10-59)
src/test/java/org/cryptomator/siv/CMacTest.java (1)
src/main/java/org/cryptomator/siv/CMac.java (1)
CMac(23-191)
src/main/java/org/cryptomator/siv/SivEngine.java (3)
src/main/java/org/cryptomator/siv/Utils.java (1)
Utils(10-59)src/main/java/org/cryptomator/siv/CMac.java (1)
CMac(23-191)src/main/java/org/cryptomator/siv/SivProvider.java (1)
SivProvider(15-31)
src/main/java/org/cryptomator/siv/SivMode.java (2)
src/main/java/org/cryptomator/siv/UnauthenticCiphertextException.java (1)
Deprecated(8-21)src/test/java/org/cryptomator/siv/SivModeTest.java (1)
Deprecated(10-36)
src/main/java9/module-info.java (1)
src/main/java/org/cryptomator/siv/SivProvider.java (1)
SivProvider(15-31)
src/main/java/org/cryptomator/siv/UnauthenticCiphertextException.java (2)
src/main/java/org/cryptomator/siv/SivMode.java (1)
Deprecated(13-173)src/test/java/org/cryptomator/siv/SivModeTest.java (1)
Deprecated(10-36)
src/test/java/org/cryptomator/siv/SivEngineTest.java (1)
src/test/java/org/cryptomator/siv/EncryptionTestCase.java (1)
EncryptionTestCase(15-90)
🪛 ast-grep (0.40.0)
src/main/java/org/cryptomator/siv/CMac.java
[warning] 48-48: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(AES_ECB_NO_PADDING)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 48-48: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(AES_ECB_NO_PADDING)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~11-~11: Ensure spelling is correct
Context: .../compare/1.6.1...HEAD) ### Added - new lowlevel API: * `new SivEngine(key).encrypt(p...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~30-~30: Ensure spelling is correct
Context: ...ate build plugins ### Deprecated - old lowlevel API: * `new SivMode().encrypt(key, p...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
12-12: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
13-13: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
31-31: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
32-32: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
33-33: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
34-34: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
🔇 Additional comments (32)
.github/workflows/codeql-analysis.yml (1)
29-29: ✓ Distribution change aligns with best practices.Temurin is a supported distribution for actions/setup-java@v5, and migrating from AdoptOpenJDK to temurin is highly recommended to receive software and security updates. Additionally, LTS versions of Eclipse Temurin are cached on GitHub Hosted Runners and the tool cache is updated weekly, which provides performance benefits for workflow execution. This change aligns well with the PR's objective to update builds to Java 21 with temurin distribution.
.github/workflows/build.yml (2)
135-135: Verify the prerelease flag setting.Line 135 marks all releases as prerelease. For a major version bump (2.0.0), confirm whether releases should be marked as prerelease or if this should be conditional/false.
Consider whether the prerelease flag should be:
- Always
true(current state)falsefor production releases- Conditional based on version pattern (e.g.,
${{ contains(github.ref, '-') }}for alpha/beta/rc versions)
84-88: Version verification logic is sound.The version verification steps correctly extract the project version from pom.xml and compare it against the git tag, ensuring consistency before deployment to Maven Central and GitHub Packages.
Also applies to: 111-115
pom.xml (4)
7-7: Version bump to 2.0.0 is appropriate.The major version bump reflects the significant breaking change of dropping BouncyCastle and JetBrains Annotations dependencies, aligning with semantic versioning principles. This change eliminates external transitive dependencies, justifying the major bump.
319-319: Verify autoPublish behavior for autonomous deployments.Line 319 sets
autoPublishtotruein thedeploy-centralprofile. This enables automatic publishing to Maven Central without manual approval steps. Confirm that this aligns with your release process, especially for a major version bump (2.0.0).If manual approval is preferred, consider setting
autoPublishtofalseand implementing a separate approval workflow step.
173-175: Test configuration with JaCoCo and Mockito is well-structured.The surefire configuration correctly wires the JaCoCo Java agent and Mockito support via the property placeholder established in the coverage profile. The Maven coordinate reference syntax for the Mockito JAR is correct.
124-136: Build plugin additions support modernized test infrastructure.The new maven-dependency-plugin, JMH annotation processor path, and JaCoCo property integration support the modernized build approach with improved test coverage and benchmark code generation. The configuration properly chains dependencies through property placeholders.
Also applies to: 144-150, 268-270
src/main/java/org/cryptomator/siv/package-info.java (1)
4-10: Package Javadoc clearly reflects the dual API surfaceThe added description of direct
SivEngineusage and the JCESivProviderpath is clear and accurate; no changes needed.src/test/java/org/cryptomator/siv/EncryptionTestCase.java (2)
53-58: Combined key helper matches engine expectations
getKey()allocates a new array and concatenatesmacKeyfollowed byctrKey, which is consistent with the combined-key layout used in the SIV engine wrappers. This is correct and keeps internal state encapsulated.
86-89: HelpfultoString()for test diagnosticsOverriding
toString()to emitTestCase #<n>is a nice improvement for parameterized test output and failure messages.src/main/resources/META-INF/services/java.security.Provider (1)
1-1: Service registration forSivProviderlooks correctDeclaring
org.cryptomator.siv.SivProviderinMETA-INF/services/java.security.Provideris the expected way to make the provider discoverable in classpath (non‑module) environments; no issues here.src/test/java/org/cryptomator/siv/SivModeTest.java (1)
10-34: Minimal regression coverage for deprecated SivMode is appropriateMarking the test class as
@Deprecatedand explicitly labeling it via@DisplayNamewhile keeping just two straightforward round‑trip tests is a good balance: existing SivMode behavior is still guarded without investing more in a deprecated API. No changes needed from my side.src/test/java/org/cryptomator/siv/UtilsTest.java (2)
12-27: shiftLeft test vectors look correct and nicely cover carry propagationThe four cases match the big-endian 1‑bit left shift semantics of
Utils.shiftLeft; no changes needed.
29-54: dbl test coverage is solid for CMAC subkey generationThe selected 16‑byte inputs (including all‑zero) exercise both branches of the doubling logic and implicitly validate
shiftLeft’s carry handling; this is good coverage for RFC‑4493 style doubling.src/main/java9/module-info.java (1)
1-7: Module descriptor correctly exports API and registers the security providerThe module exports only the public API package and declares
SivProvideras ajava.security.Providerservice, which aligns with JPMS + JCA provider best practices. No changes needed; just ensure you verify at runtime that the provider is discoverable when the JAR is placed on the module path.src/test/java/org/cryptomator/siv/SivCipherTest.java (1)
31-106: Mode, padding, key‑size, and output‑size tests align with AES‑SIV expectationsThe parameterized tests around
engineSetMode,engineSetPadding, key sizes, andengineGetOutputSize(for both encrypt and decrypt) exercise the main configuration and sizing edge cases expected from the SPI; they look consistent with the SivEngine key requirements and SIV layout (tag + ciphertext).src/test/java/org/cryptomator/siv/SivModeBenchmark.java (1)
32-55: SivEngine‑based benchmark wiring looks correct and exercises realistic sizesThe benchmark now parameterizes plaintext size via
@Param, re‑initializes key, SivEngine, and data per trial, and runs a full encrypt/decrypt round‑trip with an assertion to guard against regressions. This is a good smoke/perf test for the new zero‑dependency engine.src/main/java/org/cryptomator/siv/SivMode.java (5)
8-14: Deprecation messaging and class-level annotation are consistentJavadoc and
@DeprecatedonSivModeclearly direct callers toSivEnginewhile keeping the façade intact, which is good for a 2.x migration path.
22-41: Single-key SecretKey encrypt wrapper: delegation & zeroization look correct
encrypt(SecretKey, ...)now:
- Validates
key.getEncoded()is non-null.- Delegates to
new SivEngine(keyBytes).encrypt(...).- Zeroes
keyBytesin afinallyblock.This preserves behavior while improving key hygiene; no issues spotted.
43-67: Dual-SecretKey encrypt wrapper correctly reuses byte[] variant and wipes material
encrypt(SecretKey ctrKey, SecretKey macKey, ...):
- Obtains both encoded keys, checks for null, and delegates to the byte[] overload.
- Clears both temporary key arrays in
finally.Behavior and ordering are consistent; nothing blocking here.
92-116: Single-key SecretKey decrypt wrapper: exception mapping and zeroization preserve legacy API
decrypt(SecretKey, ...):
- Validates
key.getEncoded()is non-null.- Delegates to
SivEngine.decrypt.- Translates
AEADBadTagExceptioninto the deprecatedUnauthenticCiphertextExceptionwith a clear message.- Zeroes
keyBytesinfinally.This maintains the old checked-exception surface while using the new engine, which is the right trade-off for a deprecated façade.
118-144: Dual-SecretKey decrypt wrapper: consistent handling of key encoding and cleanupThe dual-
SecretKeydecrypt overload mirrors the encrypt path:
- Enforces non-null encoded keys.
- Delegates to the byte[] decrypt overload where AEAD-to-
UnauthenticCiphertextExceptionmapping occurs.- Clears both temporary key arrays in the
finallyblock.Looks consistent and safe.
src/test/java/org/cryptomator/siv/SivEngineTest.java (10)
31-45: Parameter-validation tests are well targeted to SivEngine’s contractThe
ParameterValidationnested class exercises:
- Invalid key lengths via a parameterized test.
- Decrypt with non-block-multiple ciphertext producing
IllegalBlockSizeException.This is a good front-line defense against accidental misuse of the engine API.
57-89: AAD count and short-buffer tests meaningfully stress edge conditionsThe tests for:
- AAD element-count limits on both encrypt and decrypt.
- Insufficient output buffer in
encrypt(plaintext, output, offset)expectingShortBufferException.These are useful for catching regression in bounds checking; the chosen sizes (127 AAD entries, 95-byte buffer vs. 96-byte need) are clear and intentional.
92-101: Basic round-trip test across key sizes is straightforward and sufficient
testEncryptionAndDecryption(int keylen)validates a simple encrypt/decrypt round trip for 32/48/64-byte keys. This nicely complements the more detailed vector-based tests.
103-210: RFC 5297 Vector 1 tests thoroughly cover CTR, S2V, SIV encrypt/decrypt, and failure modesThe
RfcTestVector1group:
- Verifies
computeCtr,s2v,encrypt, anddecryptagainst the appendix A.1 vector.- Includes negative tests for wrong key, corrupted ciphertext, and truncated ciphertext (expecting
AEADBadTagExceptionvs.IllegalBlockSizeExceptionappropriately).This is excellent coverage for both correctness and error signaling of the core SIV operations.
212-313: RFC 5297 Vector 2 tests validate multi-AAD and nonce handlingThe
RfcTestVector2nested tests check:
computeCtrwith a non-trivial counter input.- Full SIV encrypt/decrypt with two AAD values plus a nonce.
Given how subtle multi-AAD + nonce combinations can be, this is a valuable set of regression tests.
331-347: Encrypt/decrypt parameterized tests over generated cases nicely complement RFC vectorsThe parameterized
testDecryptandtestEncryptmethods:
- Use
@FieldSource("testCases")with@TestInstance(PER_CLASS)to pull from the preloaded list.- Round-trip all generated cases and assert exact ciphertext/plaintext matches.
This is a strong regression net over a broader input space than the official RFC vectors alone.
349-367: MAC-key tampering test exercises authentication robustness per test caseIn
testDecryptFailsDueToTamperedMacKeyyou:
- Derive an index into the MAC half of the combined key using
testCaseNumber % halfKeyLen.- Flip a single bit and expect
AEADBadTagExceptionon decrypt.This is a clear and efficient way to validate that MAC key corruption is reliably detected across the generated suite.
369-385: Ciphertext tampering test correctly uses per-case variation
testDecryptFailsDueToTamperedCiphertext:
- Flips one bit in a position derived from
testCaseNumber % ciphertext.length.- Asserts
AEADBadTagExceptionfromdecrypt(...).Because each ciphertext copy is per-test-instance, there’s no cross-test contamination, and this provides broad coverage of different tamper positions.
387-419: AAD tampering loop is careful about empty elements and restores state between attemptsIn
testDecryptFailsDueToTamperedAAD:
- You skip cases without AAD entirely.
- For each non-empty AD element, you flip a bit, assert
AEADBadTagException, and then flip it back.Since
testCase.getAssociatedData()returns deep copies, mutations are confined to this test, and the explicit restoration maintains clear test intent. Nicely done.
421-461: Prepended/appended AAD tests exercise structural AAD changes and engine limitsThe prepended and appended AAD tests:
- Skip cases that would exceed the engine’s AAD-element limit.
- Construct new AAD arrays with extra empty-ish elements at the front or back.
- Expect
AEADBadTagExceptionon decrypt.These patterns are very helpful for ensuring that AAD ordering and length are strictly enforced by
SivEngine.
[deploy]
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/cryptomator/siv/Utils.java (1)
15-18: Guardpadagainst invaliddesiredLengthto avoid OOB writes
padstill assumesin.length < desiredLengthand unconditionally writesresult[in.length]. If a future caller ever passesin.length >= desiredLength(or a null input), this will blow up withArrayIndexOutOfBoundsExceptioninstead of a clear argument error.Given this is a low-level crypto primitive, it’s safer to enforce the precondition explicitly:
- // First bit 1, following bits 0. - static byte[] pad(byte[] in, int desiredLength) { - final byte[] result = Arrays.copyOf(in, desiredLength); - result[in.length] = (byte) 0x80; - return result; - } + // First bit 1, following bits 0. + static byte[] pad(byte[] in, int desiredLength) { + if (in == null || in.length >= desiredLength) { + throw new IllegalArgumentException("pad input must be non-null and shorter than desiredLength"); + } + final byte[] result = Arrays.copyOf(in, desiredLength); + result[in.length] = (byte) 0x80; + return result; + }This keeps the current behavior for valid callers while turning latent OOBs into explicit, descriptive failures.
🧹 Nitpick comments (5)
src/test/java/org/cryptomator/siv/CMacTest.java (1)
20-111: CMAC vector coverage and reset behavior look solidThe RFC 4493 + NIST vector coverage and the
engineResettest give strong confidence in the CMac implementation. The HexConverter and hex helpers are fine as-is; if you ever reuse them more broadly, you might optionally add an assertion that hex strings have even length to fail fast on malformed test data, but that’s not required here.src/test/java/org/cryptomator/siv/SivCipherTest.java (1)
70-107: Comprehensive SivCipher SPI tests; consider one provider-levelCiphertestThe SPI tests cover the important behaviors (mode/padding checks, key sizes, output size, AAD handling, wrap/unwrap, reuse, determinism) very well. To catch any future regressions around
engineGetOutputSizeand buffering semantics, you might optionally add aSivProvider-based test that usesCipher.getInstance("AES/SIV/NoPadding"), performs one or moreupdate()calls, then callsgetOutputSize()followed bydoFinal(), asserting that noShortBufferExceptionoccurs and the plaintext round-trips correctly. That would complement these SPI-level tests nicely.Also applies to: 108-149
src/main/java/org/cryptomator/siv/SivEngine.java (2)
75-118:encrypt(..., output, outputOffset, ...)now correctly honors the offset; consider explicit offset validationThe buffer-based
encryptoverload now (a) guards against integer overflow, (b) checks remaining capacity relative tooutputOffset, and (c) passesoutputOffsetthrough both the IV copy andcomputeCtrcall. This fixes the earlier bug where non-zero offsets were ignored.If you want clearer error semantics for obviously invalid offsets (e.g., negative or greater than
output.length), you could add an explicit argument check and reserveShortBufferExceptionpurely for “too small buffer” cases:public int encrypt(byte[] plaintext, byte[] output, int outputOffset, byte[]... associatedData) throws ShortBufferException { + if (outputOffset < 0 || outputOffset > output.length) { + throw new IllegalArgumentException("outputOffset out of range"); + } + // Check if plaintext length will cause overflows if (plaintext.length > (Integer.MAX_VALUE - IV_LENGTH)) { throw new IllegalArgumentException("Plaintext is too long"); } @@ - System.arraycopy(iv, 0, output, outputOffset, IV_LENGTH); - return IV_LENGTH + computeCtr(plaintext, 0, plaintext.length, iv, 0, IV_LENGTH, output, outputOffset + IV_LENGTH); + System.arraycopy(iv, 0, output, outputOffset, IV_LENGTH); + return IV_LENGTH + computeCtr(plaintext, 0, plaintext.length, iv, 0, IV_LENGTH, output, outputOffset + IV_LENGTH); }This is a behavioral polish, not a correctness fix.
171-188: Optional: avoid per-call IV allocation incomputeCtr
computeCtrcurrently doesArrays.copyOfRangeon the IV for every call, which allocates a fresh 16-byte array per encrypt/decrypt. If you want to further trim allocations, you can reuse a single scratch IV buffer per engine instance (the class is already documented as non–thread-safe):public final class SivEngine { @@ - private final Mac cmac; - private final Cipher ctrCipher; + private final Mac cmac; + private final Cipher ctrCipher; + // Scratch buffer for adjusted IV in CTR mode (not thread-safe by design) + private final byte[] adjustedIv = new byte[IV_LENGTH]; @@ - private int computeCtr(byte[] input, int inOff, int inLen, final byte[] iv, int ivOff, int ivLen, byte[] output, int outputOffset) throws ShortBufferException { - // clear out the 31st and 63rd (rightmost) bit: - assert ivLen == IV_LENGTH; - final byte[] adjustedIv = Arrays.copyOfRange(iv, ivOff, ivOff + ivLen); + private int computeCtr(byte[] input, int inOff, int inLen, final byte[] iv, int ivOff, int ivLen, byte[] output, int outputOffset) throws ShortBufferException { + // clear out the 31st and 63rd (rightmost) bit: + assert ivLen == IV_LENGTH; + System.arraycopy(iv, ivOff, adjustedIv, 0, ivLen); adjustedIv[8] = (byte) (adjustedIv[8] & 0x7F); adjustedIv[12] = (byte) (adjustedIv[12] & 0x7F); @@ - ctrCipher.init(Cipher.ENCRYPT_MODE, ctrKey, new IvParameterSpec(adjustedIv)); + ctrCipher.init(Cipher.ENCRYPT_MODE, ctrKey, new IvParameterSpec(adjustedIv));Functionality remains the same, but you avoid a small but hot-path allocation.
src/test/java/org/cryptomator/siv/SivEngineTest.java (1)
323-471: Generated test cases and tamper scenarios are strong; consider a clearer failure for missingtestcases.txtThe
GeneratedTestCasesnested class:
- Uses
@TestInstance(PER_CLASS)+@BeforeAlland@FieldSource("testCases")to load and reuse a list ofEncryptionTestCases.- Employs try‑with‑resources to correctly close
InputStream,InputStreamReader, andBufferedReader, addressing the prior “potential input resource leak” warning.- Exercises positive decrypt/encrypt as well as a wide range of tampering scenarios (MAC key, ciphertext, AAD modified, prepended, and appended), which is excellent for robustness.
One optional robustness tweak: if
/testcases.txtis ever missing from the classpath,getResourceAsStreamwill returnnull, leading to aNullPointerExceptionwhen constructing theInputStreamReader. You could turn that into a more explicit failure or assumption:@BeforeAll public void loadTestCases() throws IOException { - try (InputStream in = EncryptionTestCase.class.getResourceAsStream("/testcases.txt"); + try (InputStream in = EncryptionTestCase.class.getResourceAsStream("/testcases.txt"); + // Fail fast with a clearer message if the resource is missing + ) { + Assertions.assertNotNull(in, "Resource /testcases.txt not found on classpath"); - Reader reader = new InputStreamReader(in, StandardCharsets.US_ASCII); + Reader reader = new InputStreamReader(in, StandardCharsets.US_ASCII); BufferedReader bufferedReader = new BufferedReader(reader)) { this.testCases = bufferedReader.lines().map(EncryptionTestCase::fromLine).collect(Collectors.toList()); } }or, if you prefer skipping tests when the resource is unavailable, use
Assumptions.assumeTrue(in != null, "...")instead ofassertNotNull.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.md(1 hunks)README.md(3 hunks)src/main/java/org/cryptomator/siv/CMac.java(1 hunks)src/main/java/org/cryptomator/siv/SivCipher.java(1 hunks)src/main/java/org/cryptomator/siv/SivEngine.java(1 hunks)src/main/java/org/cryptomator/siv/Utils.java(1 hunks)src/test/java/org/cryptomator/siv/CMacTest.java(1 hunks)src/test/java/org/cryptomator/siv/SivCipherTest.java(1 hunks)src/test/java/org/cryptomator/siv/SivEngineTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧬 Code graph analysis (4)
src/main/java/org/cryptomator/siv/CMac.java (1)
src/main/java/org/cryptomator/siv/Utils.java (1)
Utils(10-59)
src/main/java/org/cryptomator/siv/SivEngine.java (3)
src/main/java/org/cryptomator/siv/Utils.java (1)
Utils(10-59)src/main/java/org/cryptomator/siv/CMac.java (1)
CMac(23-194)src/main/java/org/cryptomator/siv/SivProvider.java (1)
SivProvider(15-31)
src/test/java/org/cryptomator/siv/SivEngineTest.java (1)
src/test/java/org/cryptomator/siv/EncryptionTestCase.java (1)
EncryptionTestCase(15-90)
src/test/java/org/cryptomator/siv/CMacTest.java (1)
src/main/java/org/cryptomator/siv/CMac.java (1)
CMac(23-194)
🪛 ast-grep (0.40.0)
src/main/java/org/cryptomator/siv/CMac.java
[warning] 48-48: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(AES_ECB_NO_PADDING)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 48-48: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(AES_ECB_NO_PADDING)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test
🔇 Additional comments (9)
CHANGELOG.md (1)
8-35: All previous review issues have been resolved and changes are well-structured.The unreleased section correctly documents the major API changes:
- Spelling corrections ("low-level" hyphenation) applied consistently
- Markdown list indentation fixed to 2 spaces
- Code block language specifier added for syntax highlighting
- Content accurately reflects the PR objectives: new SivEngine/SivCipher APIs, BouncyCastle removal, and SivMode deprecation
The organization follows Keep a Changelog conventions with clear Added, Changed, and Deprecated subsections, and version headers include correct comparison ranges.
src/main/java/org/cryptomator/siv/CMac.java (1)
25-69: CMAC core logic matches RFC 4493 semanticsSubkey derivation (
L→K1/K2viadbl), block processing, padding of the final block, and the reset behavior all look correct and align with RFC 4493.engineReset()at the end ofengineInitensures stale message state doesn’t leak across reinitializations. No changes needed from my side here.Also applies to: 71-138
src/main/java/org/cryptomator/siv/SivEngine.java (3)
46-73: Key splitting and primitive initialization look correctThe constructor’s key-length validation, split into MAC/CTR subkeys, and initialization of CMAC and CTR cipher are consistent with AES-SIV requirements and fail fast with clear exceptions if the environment is misconfigured. No changes needed here.
120-157: Decrypt path and tag verification are robust and side‑channel awareThe decrypt method correctly:
- Rejects inputs shorter than one block.
- Decrypts using CTR with the SIV as IV.
- Recomputes S2V over the resulting plaintext and associated data.
- Compares the stored tag and recomputed tag in a fixed-time loop and zeroizes the plaintext buffer before throwing on mismatch.
This aligns well with AEAD expectations and SIV’s misuse-resistant design; no changes needed.
190-219: S2V implementation matches RFC 5297 and reuses CMAC efficiently
s2vcorrectly enforces the AD count limit, computesD0 = CMAC(K, 0^128), iteratesD_i = dbl(D_{i-1}) XOR CMAC(K, S_i)for associated data, and then applies the “long” (≥16 bytes) vs “short” (<16 bytes) plaintext cases usingxor,dbl, andpadas per RFC 5297.Reusing a single
Macinstance and relying ondoFinalto reset internal state is a nice allocation win and is well-covered by the RFC vector tests. No changes needed.src/test/java/org/cryptomator/siv/SivEngineTest.java (4)
36-90: Parameter validation tests cover key and input preconditions wellThe
ParameterValidationnested class nicely exercises invalid key lengths, too-short ciphertext, excessive associated-data count, and insufficient output buffer scenarios. This closely matches the engine’s guards and should prevent regressions in argument checking.
92-101: Basic encrypt/decrypt round‑trip across all supported key sizesThe
testEncryptionAndDecryptionparameterized test verifies a simple round-trip for 32/48/64-byte keys, ensuring all supported key sizes behave consistently. This is a good sanity check on top of the vector tests.
103-218: RFC 5297 vector 1 tests thoroughly exercise core SIV behaviorThe
RfcTestVector1suite:
- Verifies CTR output (
testComputeCtr), S2V tag (testS2v), and full SIV encrypt/decrypt.- Includes
testSivEncryptWithOffset, which specifically validates the buffer-offset encrypt API.- Covers failure modes for invalid key, invalid ciphertext bytes, and truncated ciphertext.
This gives excellent coverage of the first RFC appendix and directly guards against regressions in the SIV engine logic and offset handling.
220-321: RFC 5297 vector 2 tests validate multi‑AAD and nonce handling
RfcTestVector2checks CTR, encrypt, and decrypt against the second official vector set, including multiple associated data items and a nonce. This confirms the multi-AAD and longer-message behavior of the implementation. No changes needed.
[deploy]
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main/java/org/cryptomator/siv/CMac.java (1)
46-67: engineInit now correctly clears per-message state and derives CMAC subkeys per RFC 4493The added
engineReset()at the start ofengineInit(Line 48) ensures any buffered message state (bufferPos,msgLen,x,y,buffer) is cleared when aCMacinstance is re‑initialized with a new key, fixing the stale‑state issue from earlier revisions. Subkey derivation (L→k1viadbl, thenk2viadbl(k1.clone())) matches RFC 4493 section 2.3, and the temporaryLis zeroed in thefinallyblock.Using
AES/ECB/NoPaddinghere is appropriate: it is only used as a raw AES block primitive for CMAC, not as an encryption mode for multi‑block data, so the static‑analysis warning about ECB can be safely ignored.src/main/java/org/cryptomator/siv/SivCipher.java (1)
54-63:engineGetOutputSizeviolates JCE contract by riskingNullPointerExceptioninstead ofIllegalStateExceptionThe JCE Cipher.getOutputSize(int) contract requires throwing
IllegalStateExceptionwhen the Cipher object has not been initialized.The current code has a contract violation:
inputBufferis declared without initialization (line 33), so it remainsnulluntilengineInitsets it toEMPTY(line 84). IfengineGetOutputSizeis called beforeengineInit, accessinginputBuffer.lengththrowsNullPointerExceptionrather than the requiredIllegalStateException.The suggested fix is correct—check initialization state first and throw
IllegalStateExceptionexplicitly:@Override protected int engineGetOutputSize(int inputLen) { + if (key == null) { + throw new IllegalStateException("Cipher not initialized"); + } + int buffered = (inputBuffer != null) ? inputBuffer.length : 0; + int totalLen = buffered + inputLen; if (opmode == Cipher.ENCRYPT_MODE || opmode == Cipher.WRAP_MODE) { - return 16 + inputBuffer.length + inputLen; + return 16 + totalLen; } else if (opmode == Cipher.DECRYPT_MODE || opmode == Cipher.UNWRAP_MODE) { - return inputBuffer.length + inputLen - 16; + return totalLen - 16; } else { throw new IllegalStateException("Invalid opmode " + this.opmode); } }
🧹 Nitpick comments (5)
src/main/java/org/cryptomator/siv/CMac.java (2)
69-101: Streaming update + finalization logic look correct; consider simplifying the complete‑block flagThe interplay of
engineUpdate(both overloads),processBlock(), andengineDoFinal()yields the expected CMAC behavior for all chunkings I can think of (single call, many smallengineUpdatecalls, and arbitrary mixes):
- Full blocks before the last one are processed via
processBlock()as new data arrives.- At
engineDoFinal()thebufferalways holds exactly the last complete block (bufferPos == 16) or a partial block (0 < bufferPos < 16), never an empty block for a non‑empty message.- The
flagconditionmsgLen > 0 && bufferPos % BLOCK_SIZE == 0therefore behaves as intended: complete last block → useK1, otherwise pad and useK2.For readability, you could consider changing the flag to
boolean flag = msgLen > 0 && bufferPos == BLOCK_SIZE;and optionally add a brief comment thatbufferPoscannot be0whenmsgLen > 0, so future readers don’t have to reason about the modulo case.Also applies to: 103-136
159-191: Staticcreate/taghelpers are convenient; consider explicit null/length contractsThe
create(byte[] key)andtag(byte[] key, byte[] message)helpers are a nice zero‑dependency way to use CMAC directly from within the library. Right now,nullkeys or messages will triggerNullPointerExceptions viakey.lengthorengineUpdate, which is acceptable but a bit implicit.If you want a slightly stricter API, you could:
- Add explicit
Objects.requireNonNull(key, "key")/Objects.requireNonNull(message, "message"), and/or- Mention in the Javadoc that
NullPointerExceptionis thrown fornullarguments, alongside the existingIllegalArgumentExceptionfor invalid key lengths.src/main/java/org/cryptomator/siv/SivCipher.java (3)
75-85: Key length validation is correct; consider handlingkey.getEncoded() == nulland zeroing old keysThe key length check against 32/48/64 bytes (256/384/512 bits) is appropriate for AES‑SIV. Two optional hardening tweaks:
- Some
Keyimplementations may returnnullfromgetEncoded(). In that case, the current code will throw aNullPointerExceptionwhen accessingkeybytes.length. You could turn that into a clearerInvalidKeyException:byte[] keybytes = key.getEncoded(); -if (keybytes.length != 64 && keybytes.length != 48 && keybytes.length != 32) { +if (keybytes == null || (keybytes.length != 64 && keybytes.length != 48 && keybytes.length != 32)) { throw new InvalidKeyException("Key length must be 256, 384, or 512 bits."); }
- When re‑initializing the cipher with a new key, you might optionally zero out the previous
this.keyarray before overwriting it to reduce key material lifetime in memory (guarding againstthis.key == nullon the first init).These are not functional issues, just defense‑in‑depth improvements.
110-122: Input buffering viaArrays.copyOfis correct but can be quadratic for many small updatesThe
engineUpdateimplementation that repeatedly does:int oldLen = inputBuffer.length; inputBuffer = Arrays.copyOf(inputBuffer, oldLen + inputLen); System.arraycopy(input, inputOffset, inputBuffer, oldLen, inputLen);is functionally fine for SIV (you need the full message at
doFinaltime), and will perform well if callers typically pass reasonably sized chunks.If
engineUpdateis used with very many tiny fragments (e.g., 1–few bytes at a time), this pattern becomes O(n²) due to repeated copying of the accumulated buffer. If that usage pattern is plausible, consider an internalByteArrayOutputStreamor chunkedList<byte[]>accumulator that only flattens once inengineDoFinal.Given the SIV use case, this is probably acceptable as‑is, so this is just a performance note rather than a required change.
140-175: ---Move state cleanup into
finallyblock to ensure robustness under exceptionsThe JCE Cipher API specifies that if an exception is thrown by
doFinal, the Cipher "may need to be reset" before reuse, and provider implementations should ensure per-message state is cleared/reset on completion and exception paths are handled to avoid inconsistent state.The current code clears
inputBufferandaadonly on the happy path. Ifsiv.encrypt()orsiv.decrypt()throws, those lines are skipped, leaving stale state. Wrapping the encrypt/decrypt logic in atry-finallyblock (as suggested in the diff) ensures cleanup always runs, making the implementation more defensive and aligned with JCE best practices:- int resultLength; - SivEngine siv = new SivEngine(this.key); - byte[][] aad = this.aad.toArray(new byte[this.aad.size()][]); - if (this.opmode == Cipher.ENCRYPT_MODE || this.opmode == Cipher.WRAP_MODE) { - resultLength = siv.encrypt(inputBuffer, output, outputOffset, aad); - } else if (this.opmode == Cipher.DECRYPT_MODE || this.opmode == Cipher.UNWRAP_MODE) { - // for security reasons we can't write into output directly before checking integrity: - byte[] plaintext = new byte[0]; - try { - plaintext = siv.decrypt(inputBuffer, aad); - System.arraycopy(plaintext, 0, output, outputOffset, plaintext.length); - resultLength = plaintext.length; - } finally { - Arrays.fill(plaintext, (byte) 0x00); - } - } else { - throw new IllegalStateException("Invalid opmode " + this.opmode); - } - - // reset internal state: - this.inputBuffer = EMPTY; - this.aad.clear(); - return resultLength; + int resultLength; + try { + SivEngine siv = new SivEngine(this.key); + byte[][] aadArr = this.aad.toArray(new byte[this.aad.size()][]); + if (this.opmode == Cipher.ENCRYPT_MODE || this.opmode == Cipher.WRAP_MODE) { + resultLength = siv.encrypt(inputBuffer, output, outputOffset, aadArr); + } else if (this.opmode == Cipher.DECRYPT_MODE || this.opmode == Cipher.UNWRAP_MODE) { + // for security reasons we can't write into output directly before checking integrity: + byte[] plaintext = new byte[0]; + try { + plaintext = siv.decrypt(inputBuffer, aadArr); + System.arraycopy(plaintext, 0, output, outputOffset, plaintext.length); + resultLength = plaintext.length; + } finally { + Arrays.fill(plaintext, (byte) 0x00); + } + } else { + throw new IllegalStateException("Invalid opmode " + this.opmode); + } + } finally { + // reset internal state regardless of success + this.inputBuffer = EMPTY; + this.aad.clear(); + } + return resultLength;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/cryptomator/siv/CMac.java(1 hunks)src/main/java/org/cryptomator/siv/SivCipher.java(1 hunks)src/main/java/org/cryptomator/siv/Utils.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/cryptomator/siv/Utils.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/org/cryptomator/siv/CMac.java (1)
src/main/java/org/cryptomator/siv/Utils.java (1)
Utils(10-62)
🪛 ast-grep (0.40.0)
src/main/java/org/cryptomator/siv/CMac.java
[warning] 49-49: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(AES_ECB_NO_PADDING)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 49-49: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(AES_ECB_NO_PADDING)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test
# Conflicts: # .github/workflows/build.yml [deploy]
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pom.xml (2)
124-136: Add explicit version to maven-dependency-plugin for reproducibility.The maven-dependency-plugin lacks an explicit version declaration. For reproducible builds, all plugin versions should be pinned.
<plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-dependency-plugin</artifactId> + <version>3.11.1</version> <executions> <execution> <id>jar-paths-to-properties</id> <phase>validate</phase> <goals> <goal>properties</goal> </goals> </execution> </executions> </plugin>
332-332: Flag autoPublish=true for operational impact review.The central-publishing-maven-plugin's
autoPublishis now enabled. This automatically publishes artifacts to Maven Central without manual intervention. Ensure this change is intentional and documented in release notes, as it affects the release workflow.Consider whether automated publishing without a manual gate is appropriate for this major version bump (2.0.0). If you want to retain a manual approval step, set
autoPublishtofalseand require manual invocation.<central-publishing-maven-plugin> ... - <autoPublish>true</autoPublish> + <autoPublish>false</autoPublish> </central-publishing-maven-plugin>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(1 hunks)pom.xml(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyse
- GitHub Check: Build and Test
🔇 Additional comments (4)
pom.xml (4)
52-52: Verify surefire property is populated when coverage profile is not active.The
surefire.jacoco.argsproperty (line 52) defaults to empty, but it's referenced in the surefire argLine (line 174). When the coverage profile is not active, this will expand to an empty string, which is safe but should be verified to have no unintended side effects on test execution.
144-150: Confirm JMH annotation processor is required for test compilation.The added annotationProcessorPaths for
jmh-generator-annprocessis necessary for JMH benchmark classes to be processed during test compilation. Verify that this aligns with the benchmark implementations and that the JMH version is consistent across build and test dependencies.
173-175: Verify argLine property substitution and mockito javaagent availability.The argLine uses
@{surefire.jacoco.args}(deferred evaluation) and${org.mockito:mockito-core:jar}(dependency artifact path). The latter relies on maven-dependency-plugin's properties goal to generate theorg.mockito:mockito-core:jarproperty. Confirm that:
- The mockito JAR path is correctly resolved when the dependency-plugin executes in the validate phase.
- The argLine does not fail when mockito-core is missing or the property is unresolved.
- Tests pass consistently across CI/CD environments.
268-270: JaCoCo property mapping ensures code coverage integration.The
surefire.jacoco.argsproperty name mapping correctly wires JaCoCo's agent arguments into surefire, enabling code coverage collection during test execution when the coverage profile is active.
SailReal
left a comment
SailReal
left a comment
There was a problem hiding this comment.
Tested it on Android, works fine 👍
This drops all dependencies, internalizes the CMAC implementation and improves performance by avoiding byte[] allocations.
Compared to the baseline, allocations are reduced by 60%, which results in a speedup of 15-25 % depending on input size: