gnodet-bot commented on code in PR #26725:
URL: https://github.com/apache/camel/pull/26725#discussion_r4080055134
##########
components/camel-crypto/src/main/java/org/apache/camel/converter/crypto/CryptoDataFormat.java:
##########
@@ -166,8 +186,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:
🔴 **`AUTHENTICATION_FAILED` is thrown even when `shouldAppendHMAC=false` —
davsclaus's finding is still open.**
This catch block is unconditional. When `shouldAppendHMAC=false`, the
anonymous HMACAccumulator's `decryptUpdate` just writes to the output stream —
no MAC verification happens. A `BadPaddingException` surfacing here is
corruption or a key mismatch, not an authentication failure. Reporting it as
`AUTHENTICATION_FAILED` misdescribes the error and discards the original cause
entirely.
Gate on `shouldAppendHMAC`:
```suggestion
} catch (IOException e) {
if (shouldAppendHMAC && 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);
}
throw e;
```
##########
components/camel-crypto/src/main/java/org/apache/camel/converter/crypto/CryptoDataFormat.java:
##########
@@ -121,9 +126,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) {
Review Comment:
🔴 **`algorithmParameterSpec` + `inline`: IV is generated and written but
never used in the cipher — davsclaus's finding is still open.**
`if (iv == null && inline)` generates a fresh IV, but `initializeCipher`
gives `algorithmParameterSpec` priority:
```java
if (algorithmParameterSpec != null) {
cipher.init(mode, key, algorithmParameterSpec); // iv is ignored
} else if (iv != null) {
cipher.init(mode, key, new IvParameterSpec(iv));
}
```
The result is identical ciphertext on every call — the IV written into the
message is window dressing. Before this PR, that configuration threw `Inlining
cannot be performed, as no initialization vector was specified`, which at least
told the caller something was wrong.
The fix is to guard IV generation against `algorithmParameterSpec`:
```suggestion
if (iv == null && inline && algorithmParameterSpec == null) {
```
Alternatively, make `algorithmParameterSpec` + `inline` a hard error: if the
caller has set `algorithmParameterSpec`, they are managing the IV externally
and inlining is not meaningful.
##########
components/camel-crypto/src/test/java/org/apache/camel/converter/crypto/CryptoDataFormatIvAndFailureTest.java:
##########
@@ -0,0 +1,165 @@
+/*
+ * 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 — still present, OpenRewrite will rewrite it and CI will fail.**
Add `import java.util.Arrays;` to the import section and use the simple name:
```suggestion
assertFalse(Arrays.equals(first, second),
```
##########
components/camel-crypto/src/test/java/org/apache/camel/converter/crypto/CryptoDataFormatIvAndFailureTest.java:
##########
@@ -0,0 +1,165 @@
+/*
+ * 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);
+
+ // The MAC is written through the CipherOutputStream during
marshal, so it is encrypted inside the
+ // ciphertext, not appended in the clear. The last wire byte is
therefore the final ciphertext block, and
+ // flipping it makes that block no longer decrypt to valid PKCS5
padding - the BadPaddingException path,
+ // distinct from the bad-MAC path exercised just below.
+ 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:
🟠**FQCN — same issue.**
Add `import java.io.ByteArrayInputStream;` and use the simple name:
```suggestion
Object result = format.unmarshal(exchange, new
ByteArrayInputStream(body));
```
--
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]
