davsclaus commented on code in PR #26725:
URL: https://github.com/apache/camel/pull/26725#discussion_r4071649745
##########
components/camel-crypto/src/main/java/org/apache/camel/converter/crypto/CryptoDataFormat.java:
##########
@@ -121,9 +123,24 @@ private Cipher initializeCipher(int mode, Key key, byte[]
iv) throws Exception {
return cipher;
}
+ /**
+ * Upper bound on the length of an inlined initialization vector read from
the stream. A JCE initialization vector
+ * is at most a cipher block, so this is generous; the bound exists
because the length is read from the message and
+ * used directly to size an allocation.
+ */
+ private static final int MAX_INLINE_IV_LENGTH = 1024;
+
+ private static final SecureRandom SECURE_RANDOM = new SecureRandom();
+
@Override
public void marshal(Exchange exchange, Object graph, OutputStream
outputStream) throws Exception {
byte[] iv = getInitializationVector(exchange);
+ if (iv == null && inline) {
+ // The whole point of inlining is that the IV travels with the
message, so there is no reason to make
+ // the caller supply a fixed one - and requiring it is what used
to push users into reusing a single IV
+ // across every message.
+ iv = generateInitializationVector();
Review Comment:
๐ด **This silently reintroduces IV reuse when `algorithmParameterSpec` is
set.**
`initializeCipher` gives `algorithmParameterSpec` precedence over `iv`:
```java
if (algorithmParameterSpec != null) {
cipher.init(mode, key, algorithmParameterSpec);
} else if (iv != null) {
cipher.init(mode, key, new IvParameterSpec(iv));
}
```
So with both `algorithmParameterSpec` and
`shouldInlineInitializationVector(true)` configured, a fresh IV is generated
and **written into every message**, but never used. Probed on this branch:
```
inlined IV msg1 = c6a7fbe2c4d04d96a2e44b74db9b2372
inlined IV msg2 = 484cfbf15f49fce715cbc88fc5d5ce78
ciphertext identical across messages? true
```
Same plaintext, byte-identical ciphertext, behind an IV that *looks*
per-message. Decryption ignores the inlined IV too (same precedence), so it
round-trips and nothing ever errors. Before this PR that configuration threw
`Inlining cannot be performed, as no initialization vector was specified` โ
loud and correct.
The same guard also fixes the ECB case, where the generated IV now reaches
`new IvParameterSpec(iv)` and produces `InvalidAlgorithmParameterException: ECB
mode cannot use IV` instead of the old clear message.
```suggestion
iv = generateInitializationVector();
}
if (iv == null && inline && algorithmParameterSpec != null) {
// the parameter spec wins in initializeCipher, so a generated
vector would be written into the
// message without being used - which looks like a per-message
vector while every message is
// encrypted identically. Keep failing loudly instead.
throw new IllegalStateException(
"Inlining cannot be performed when an
algorithmParameterSpec is configured, as the spec is"
+ " used instead of the
initialization vector");
}
```
##########
components/camel-crypto/src/main/java/org/apache/camel/converter/crypto/CryptoDataFormat.java:
##########
@@ -166,8 +183,20 @@ public Object unmarshal(final Exchange exchange, final
InputStream encryptedStre
byte[] buffer = new byte[bufferSize];
hmac.attachStream(osb);
int read;
- while ((read = cipherStream.read(buffer)) >= 0) {
- hmac.decryptUpdate(buffer, read);
+ try {
+ while ((read = cipherStream.read(buffer)) >= 0) {
+ hmac.decryptUpdate(buffer, read);
+ }
+ } catch (IOException e) {
+ if (e.getCause() instanceof GeneralSecurityException) {
+ // CipherInputStream surfaces bad padding as an
IOException wrapping
+ // BadPaddingException, while a bad MAC surfaces from
validate() below. Reporting the two
+ // differently is exactly what lets a caller who can
submit ciphertext and watch the
+ // outcome tell them apart, which is the
padding-oracle distinguisher. Report the same
+ // authentication failure for both.
+ throw new
IllegalStateException(HMACAccumulator.AUTHENTICATION_FAILED);
Review Comment:
๐ด **Reported even when nothing is authenticating.**
`getMessageAuthenticationCode` returns a no-op accumulator when
`shouldAppendHMAC=false` โ its `validate()` is empty. This rewrite is
unconditional, so an operator who deliberately turned the MAC off now gets:
```
PROBE1 shouldAppendHMAC=false, bad padding -> IllegalStateException: Message
authentication failed
```
There is no MAC and no authentication; the real cause is corruption or a key
mismatch. The message misdescribes it, and because the cause is dropped
entirely there is nothing left to debug from.
Suggest gating on `shouldAppendHMAC` and keeping the original cause
reachable at debug level:
```suggestion
if (shouldAppendHMAC) {
LOG.debug("Reporting cipher failure as an
authentication failure", e);
throw new
IllegalStateException(HMACAccumulator.AUTHENTICATION_FAILED);
}
```
##########
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:
๐ก Nit: this builds a full `Cipher` on every marshal purely to read
`getBlockSize()`, which is constant for a given `algorithm`/`cryptoProvider`.
Caching it in a field (computed lazily, or in `doStart()`) would take the
allocation off the per-message path.
##########
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),
Review Comment:
๐ FQCN โ CLAUDE.md ยง *Import Style* requires an import and the simple name,
in test code too. OpenRewrite shortens this during the build, so CI's
uncommitted-changes check will fail on it.
Add `import java.util.Arrays;` and:
```suggestion
assertFalse(Arrays.equals(first, second),
```
##########
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;
+
+ // corrupt a byte in the middle: padding still validates, the
appended MAC does not
+ byte[] badMac = ciphertext.clone();
+ badMac[badMac.length / 2] ^= 0x01;
+
+ String paddingFailure = failureMessage(context, format,
badPadding);
+ String macFailure = failureMessage(context, format, badMac);
+
+ assertEquals(macFailure, paddingFailure, "the two failures must be
indistinguishable");
+ assertTrue(paddingFailure.contains("authentication failed"),
"unexpected message: " + paddingFailure);
+ }
+ }
+
+ /**
+ * The inlined length is read from the message and used to size an
allocation, so it has to be bounded.
+ */
+ @Test
+ void anOversizedInlinedInitializationVectorLengthIsRejected() throws
Exception {
+ Key key = key();
+ try (DefaultCamelContext context = new DefaultCamelContext()) {
+ context.start();
+ CryptoDataFormat decryptor = new
CryptoDataFormat("AES/CBC/PKCS5Padding", key);
+ decryptor.setShouldInlineInitializationVector(true);
+
+ // a four byte length of 0x7FFFFFFF followed by nothing
+ byte[] hostile = { 0x7F, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF };
+
+ Exception e = assertThrows(Exception.class, () ->
unmarshal(context, decryptor, hostile));
+ assertTrue(rootMessage(e).contains("is not between 0 and"),
"unexpected message: " + rootMessage(e));
+ }
+ }
+
+ @Test
+ void aRoundTripWithAStaticVectorStillWorks() throws Exception {
+ Key key = key();
+ byte[] iv = new byte[16];
+ try (DefaultCamelContext context = new DefaultCamelContext()) {
+ context.start();
+ CryptoDataFormat format = new
CryptoDataFormat("AES/CBC/PKCS5Padding", key);
+ format.setInitVector(iv);
+
+ byte[] ciphertext = marshal(context, format, PAYLOAD);
+ assertEquals(PAYLOAD, unmarshal(context, format, ciphertext));
+ assertArrayEquals(iv, format.getInitVector());
+ }
+ }
+
+ private static String failureMessage(DefaultCamelContext context,
CryptoDataFormat format, byte[] body) {
+ Exception e = assertThrows(Exception.class, () -> unmarshal(context,
format, body));
+ return rootMessage(e);
+ }
+
+ private static String rootMessage(Throwable t) {
+ while (t.getCause() != null) {
+ t = t.getCause();
+ }
+ return String.valueOf(t.getMessage());
+ }
+
+ private static byte[] marshal(DefaultCamelContext context,
CryptoDataFormat format, String payload)
+ throws Exception {
+ Exchange exchange = new DefaultExchange(context);
+ ByteArrayOutputStream out = new ByteArrayOutputStream();
+ format.marshal(exchange, payload.getBytes(StandardCharsets.UTF_8),
out);
+ return out.toByteArray();
+ }
+
+ private static String unmarshal(DefaultCamelContext context,
CryptoDataFormat format, byte[] body)
+ throws Exception {
+ Exchange exchange = new DefaultExchange(context);
+ Object result = format.unmarshal(exchange, new
java.io.ByteArrayInputStream(body));
Review Comment:
๐ Same here โ add `import java.io.ByteArrayInputStream;` and use the simple
name.
```suggestion
Object result = format.unmarshal(exchange, new
ByteArrayInputStream(body));
```
##########
components/camel-crypto/src/main/java/org/apache/camel/converter/crypto/CryptoDataFormat.java:
##########
@@ -121,9 +123,24 @@ private Cipher initializeCipher(int mode, Key key, byte[]
iv) throws Exception {
return cipher;
}
+ /**
+ * Upper bound on the length of an inlined initialization vector read from
the stream. A JCE initialization vector
+ * is at most a cipher block, so this is generous; the bound exists
because the length is read from the message and
+ * used directly to size an allocation.
+ */
+ private static final int MAX_INLINE_IV_LENGTH = 1024;
Review Comment:
๐ก Nit: these two constants sit between `initializeCipher` and `marshal`
rather than with the other fields at the top of the class (`LOG`,
`INIT_VECTOR`, `algorithm`, โฆ). Moving them up keeps the declaration order
consistent with the rest of the file.
--
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]
