Strict output buffer requirements in AES/CBC decryption cause compatibility issues with SunJCE-based code

[alevymyers]

Summary:
ACCP enforces strict buffer requirements when decrypting data using AES/CBC with padding (e.g., PKCS5Padding, PKCS7Padding). Specifically, it requires that the output buffer passed to Cipher.update() be large enough to hold the full decrypted plaintext output, including the final block that contains padding, as mentioned here in the code.

While this behavior is fully compliant with the JCA specification, it differs from the behavior of the default Java provider (SunJCE), which will buffer the final block internally and defer writing it until Cipher.doFinal() is called. SunJCE will subtract the padding when performing buffer size checks. This difference in behavior leads to compatibility issues with existing applications, including https://github.com/trinodb/trino.

Trino contains logic in the CompressingDecryptingPageDeserializer class that calls Cipher.update() during page deserialization with an output buffer sized based on assumptions consistent with SunJCE.

More specifically, it passes a buffer with a size equal to the size of the data to be decrypted, meaning it will get a java.crypto.ShortBufferException when padding is required.

Tests such as TestPagesSerde.testDeserializationWithRollover will fail due to this assumption.

This difference is not documented in the DIFFERENCES.md and it violates the statement made in the README.md

This means that it can be used as a drop in replacement for many different Java applications

The described behavior difference stems from how ACCP integrates with aws-lc native code and should be remedied. ACCP must always provide drop-in compatibility with JDK.

Reproducing the error in Trino:

1. Clone Trino and checkout
2. Build with ./mvnw clean install -DskipTests
3. Run the failing test ./mvnw test -Dtest=io.trino.execution.buffer.TestPagesSerde -pl core/trino-main

Or, the following code sample highlights the difference where ACCP will throw a ShortBufferException while SunJCE will not:

byte[] keyBytes = new byte[16];
byte[] ivBytes = new byte[16];
SecureRandom random = new SecureRandom();
random.nextBytes(keyBytes);
random.nextBytes(ivBytes);

SecretKeySpec key = new SecretKeySpec(keyBytes, "AES");
IvParameterSpec iv = new IvParameterSpec(ivBytes);

byte[] plaintext = new byte[32];
random.nextBytes(plaintext);

Cipher encryptCipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
encryptCipher.init(Cipher.ENCRYPT_MODE, key, iv);
byte[] ciphertext = encryptCipher.doFinal(plaintext);

Cipher decryptCipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
decryptCipher.init(Cipher.DECRYPT_MODE, key, iv);

int outputSize = decryptCipher.getOutputSize(ciphertext.length - 10); // buffer.size < size + padding block

byte[] outputBuffer = new byte[outputSize];

int updateLen = decryptCipher.update(ciphertext, 0, ciphertext.length, outputBuffer, 0); // ACCP throws exception here
int finalLen = decryptCipher.doFinal(outputBuffer, updateLen);

int totalLen = updateLen + finalLen;

byte[] decrypted = Arrays.copyOf(outputBuffer, totalLen);
System.out.println("Decryption successful: " + Arrays.equals(plaintext, decrypted));

[geedo0]

Hi there, thanks for reporting this discrepancy. I agree that we shouldn't have the difference if it can be avoided. I also don't like that the output buffer gets modified while reporting only a subset of it as valid output. Your proposal to buffer it in the Java layer sounds reasonable. However, we'll need to investigate to make sure there are no regressions in terms of performance/correctness.

[alevymyers]

Thank you for the response. Let me know if I can help and when there are any updates.

[geedo0]

I looked into this and unfortunately we cannot implement this without accepting a performance tradeoff in exchange for compatibility. Due to the way AWS-LC implements AES-CBC with PKCS5Padding, ACCP is required to provide a writable output buffer for the entire incoming ciphertext. This means that the only way to achieve compatibility is to allocate this additional memory for AWS-LC and copy the data in/out of it. This isn't necessarily be a deal breaker, but after discussing this with the team we decided it's not an acceptable trade-off for us today. I will document this behavioral difference in DIFFERENCES.md.

---

The reason why AWS-LC imposes this constraint can be seen here. It decrypts all of the blocks of incoming ciphertext and writes it to the provided output buffer. It then caches the final block in the context and truncates the reported output as it contains padding bytes that must be removed during the doFinal call.

In trinodb, the buffers are set based on assumptions around SunJCE's implementation details rather than querying Cipher.getOutputSize() which we set appropriately. Based on the surrounding git history, they chose to optimize performance for the specific implementation details of SunJCE rather than have a "by the book" usage of the JCE API. This could be interpretted as a bug/oversight in trinodb and I recommend reaching out to their maintainers to see what can be done to improve compatibility. There's also no specification/requirement here that requires either ACCP/SunJCE to have this specific buffering behavior. We would feel more inclined to implement this change if there were JDK docs or JCK tests that support it as a strict compatibility issue rather than an implementation detail.

[alevymyers]

Thank you for the response walking through the performance trade-offs and additional context. I understand the rationale, but in our view ACCP should remain bug-for-bug compatible with SunJCE—especially since projects like Trino depend on this exact buffering behavior.

---

I’ll draft a PR implementing my approach and include benchmarks to measure the impact. Once it’s up, we can discuss the trade-offs more concretely and decide on the best path forward.

[geedo0]

Sounds good, we're happy to review and discuss. We are amenable to making this a configurable option via System property if it resulted in the performance trade-off described.