gnodet-bot commented on code in PR #26725:
URL: https://github.com/apache/camel/pull/26725#discussion_r4069319009
##########
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:
💡 **`generateInitializationVector()` creates a disposable `Cipher` on every
`marshal()` call just to read the block size.**
For AES, the block size is always 16 regardless of mode or padding; for
other algorithms it's also fixed by the algorithm, not the key. A one-time
cached lookup (e.g. a `private volatile int cachedBlockSize` initialized on
first use, or computed once in the constructor from `algorithm`) avoids the JCE
provider lookup on every message.
```suggestion
private byte[] generateInitializationVector() throws Exception {
int blockSize = getCipherBlockSize();
byte[] iv = new byte[blockSize];
SECURE_RANDOM.nextBytes(iv);
return iv;
}
private int getCipherBlockSize() throws Exception {
// Block size is a property of the algorithm, not the key — cache it.
if (cachedBlockSize <= 0) {
Cipher cipher = cryptoProvider == null
? Cipher.getInstance(algorithm)
: Cipher.getInstance(algorithm, cryptoProvider);
int bs = cipher.getBlockSize();
cachedBlockSize = bs > 0 ? bs : 16;
}
return cachedBlockSize;
}
```
(Add `private volatile int cachedBlockSize;` to the field declarations.)
##########
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:
⚠️ **The `badPadding` test case doesn't exercise the padding-oracle fix — it
tests MAC failure instead.**
With `shouldAppendHMAC = true` (the default), the wire format is
`[ciphertext (48 bytes)][HMAC-SHA1 (20 bytes)]` for the 43-byte payload used
here. `badPadding[badPadding.length - 1] ^= 0x01` flips the last byte of the
**appended HMAC**, not the last byte of any ciphertext block. The cipher
decrypts successfully with valid padding, and `hmac.validate()` then throws
`Message authentication failed` — the exact same path as the `badMac` case.
Neither test case triggers the `catch (IOException e)` block that catches
`BadPaddingException`.
To test the actual fix, corrupt a byte within the *ciphertext* range
(indices 0–47), not the HMAC tail.
```suggestion
// corrupt a byte in the ciphertext: the last full AES block
(bytes 32–47) no longer decrypts to valid padding
byte[] badPadding = ciphertext.clone();
badPadding[ciphertext.length - 1 - /* HMAC-SHA1 length */ 20] ^=
0x01;
```
--
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]
