oscerd commented on code in PR #26725: URL: https://github.com/apache/camel/pull/26725#discussion_r4080029075
########## components/camel-crypto/src/test/java/org/apache/camel/converter/crypto/CryptoDataFormatIvAndFailureTest.java: ########## @@ -0,0 +1,162 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.converter.crypto; + +import java.io.ByteArrayOutputStream; +import java.nio.charset.StandardCharsets; +import java.security.Key; + +import javax.crypto.KeyGenerator; + +import org.apache.camel.Exchange; +import org.apache.camel.impl.DefaultCamelContext; +import org.apache.camel.support.DefaultExchange; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class CryptoDataFormatIvAndFailureTest { + + private static final String PAYLOAD = "the quick brown fox jumps over the lazy dog"; + + /** + * Inlining exists so the initialization vector travels with the message. Requiring a statically configured one as + * well is what pushed routes into reusing a single vector for every message. + */ + @Test + void inliningGeneratesAFreshInitializationVectorPerMessage() throws Exception { + Key key = key(); + try (DefaultCamelContext context = new DefaultCamelContext()) { + context.start(); + CryptoDataFormat encryptor = new CryptoDataFormat("AES/CBC/PKCS5Padding", key); + encryptor.setShouldInlineInitializationVector(true); + + byte[] first = marshal(context, encryptor, PAYLOAD); + byte[] second = marshal(context, encryptor, PAYLOAD); + + assertFalse(java.util.Arrays.equals(first, second), + "the same plaintext must not produce identical ciphertext twice"); + + CryptoDataFormat decryptor = new CryptoDataFormat("AES/CBC/PKCS5Padding", key); + decryptor.setShouldInlineInitializationVector(true); + assertEquals(PAYLOAD, unmarshal(context, decryptor, first)); + assertEquals(PAYLOAD, unmarshal(context, decryptor, second)); + } + } + + /** + * A caller who can submit ciphertext and observe the outcome must not be able to tell a padding failure from a MAC + * failure - telling them apart is what turns CBC decryption into a padding oracle. + */ + @Test + void badPaddingAndBadMacAreReportedIdentically() throws Exception { + Key key = key(); + try (DefaultCamelContext context = new DefaultCamelContext()) { + context.start(); + // a static vector, not inlining, so this exercises the failure reporting and nothing else + CryptoDataFormat format = new CryptoDataFormat("AES/CBC/PKCS5Padding", key); + format.setInitVector(new byte[16]); + + byte[] ciphertext = marshal(context, format, PAYLOAD); + + // corrupt the last byte: the final block no longer decrypts to valid padding + byte[] badPadding = ciphertext.clone(); + badPadding[badPadding.length - 1] ^= 0x01; Review Comment: Thanks for the careful look, but the test does exercise the padding path — the premise doesn't hold for this format. `shouldAppendHMAC=true`, but in `marshal` the MAC is written *through* the `CipherOutputStream` (`cipherStream.write(mac)` after `getCalculatedMac()`), so it is **encrypted inside** the ciphertext (MAC-then-encrypt), not appended in the clear. There is no cleartext HMAC tail on the wire — the last byte is the final ciphertext block. I verified this empirically: with the `catch (IOException)` padding branch temporarily emitting a distinct marker, `badPadding` (last-byte flip) → the marker (the `BadPaddingException` path), while `badMac` (mid-byte flip) → `Message authentication failed` (`hmac.validate()`). Two genuinely different paths, reported identically — which is exactly the property under test. Corrupting `ciphertext.length - 1 - 20` would instead land in a block whose padding still validates, moving `badPadding` onto the MAC path. I've added a comment to the test in d17228d explaining the MAC-then-encrypt layout so this is clear on re-review. _Claude Code on behalf of oscerd_ ########## components/camel-crypto/src/main/java/org/apache/camel/converter/crypto/CryptoDataFormat.java: ########## @@ -247,6 +281,22 @@ public byte[] getCalculatedMac() { }; } + /** + * A fresh initialization vector, sized to the cipher's block length. Only used when the vector is inlined into the + * message, so the reader takes it from the stream and nothing needs to be shared out of band. + */ + private byte[] generateInitializationVector() throws Exception { + Cipher cipher = cryptoProvider == null ? Cipher.getInstance(algorithm) : Cipher.getInstance(algorithm, cryptoProvider); + int blockSize = cipher.getBlockSize(); Review Comment: Applied in d17228d — extracted `getCipherBlockSize()` backed by a `private volatile int cachedBlockSize`, so the block size is looked up once instead of building a `Cipher` on every `marshal()`. Kept the stream-cipher `<= 0 → 16` fallback. _Claude Code on behalf of oscerd_ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
