oscerd commented on code in PR #26725:
URL: https://github.com/apache/camel/pull/26725#discussion_r4091329120
##########
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:
Fixed in bdd8116. `marshal` now throws a clear `IllegalStateException` when
`inline` is set with no static IV but an `algorithmParameterSpec` is configured
— the spec wins in `initializeCipher`, so a generated vector would be written
into the message but never used (your probe: identical ciphertext across
messages behind a per-message-looking IV). Restored the loud failure the old
code gave. Added `inliningWithAnAlgorithmParameterSpecIsRejected`, verified to
fail against the un-guarded code.
On the ECB note: that path (no spec) now surfaces
`InvalidAlgorithmParameterException: ECB mode cannot use IV` — still a loud
failure rather than silent reuse. Happy to add an ECB-specific guard too if
you'd prefer the old, clearer message there.
_Claude Code on behalf of oscerd_
##########
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:
Fixed in bdd8116. The `BadPaddingException` → `AUTHENTICATION_FAILED`
rewrite is now gated on `shouldAppendHMAC`: with the MAC off there is nothing
authenticating, so the catch rethrows the real cause (with a `LOG.debug`)
instead of misdescribing it and dropping it. The padding-oracle
indistinguishability still holds where it matters — when a MAC is appended.
Added `aCipherFailureWithoutAMacIsNotReportedAsAnAuthenticationFailure`,
verified to fail against the un-gated code.
_Claude Code on behalf of oscerd_
##########
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:
Done in bdd8116 — `MAX_INLINE_IV_LENGTH` and `SECURE_RANDOM` moved up with
`LOG`/`INIT_VECTOR` and the other class fields.
_Claude Code on behalf of oscerd_
##########
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:
Done in bdd8116 — added `import java.util.Arrays;` and use
`Arrays.equals(...)`.
_Claude Code on behalf of oscerd_
##########
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:
Done in bdd8116 — added `import java.io.ByteArrayInputStream;` and use the
simple name.
_Claude Code on behalf of oscerd_
##########
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:
Fixed in bdd8116 — `marshal` now rejects `inline` + `algorithmParameterSpec`
with a clear `IllegalStateException` (the spec-wins IV-reuse trap @davsclaus
probed), with a test that fails against the un-guarded code.
_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]
