Count buffered bytes in the AEADBaseEngine in-place input/output overlap check

Author: Arpan0995

core/src/main/java/org/bouncycastle/crypto/engines/AEADBaseEngine.java

@@ -1066,7 +1066,7 @@ protected int processEncDecBytes(byte[] input, int inOff, int len, byte[] output
         resultLength = length + m_bufPos - (forEncryption ? 0 : MAC_SIZE);
         ensureSufficientOutputBuffer(output, outOff, resultLength - resultLength % BlockSize);
         resultLength = 0;
-        if (input == output && Arrays.segmentsOverlap(inOff, len, outOff, length))
+        if (input == output && Arrays.segmentsOverlap(inOff, len, outOff, length + m_bufPos))
         {
             input = new byte[len];
             System.arraycopy(output, inOff, input, 0, len);

core/src/test/java/org/bouncycastle/crypto/test/CipherTest.java

@@ -942,5 +942,78 @@ static void testOverlapping(SimpleTest test, int keySize, int ivSize, int macSiz
         test.isTrue("fail on testing overlapping of decryption for " + cipher.getAlgorithmName(),
             Arrays.areEqual(expected, 0, blockSize * 2, data, offset, offset + blockSize * 2));
 
+        testOverlappingSplit(test, keySize, ivSize, macSize, blockSize, cipher);
+    }
+
+    /**
+     * Encrypt and decrypt in two processBytes() calls with the input and the output in the same array, the
+     * output starting at, behind or ahead of the input, and compare with a single call into a separate array.
+     * The second call writes the bytes the first one buffered as well as its own, so its overlap check has to
+     * allow for them.
+     */
+    static void testOverlappingSplit(SimpleTest test, int keySize, int ivSize, int macSize, int blockSize, AEADCipher cipher)
+        throws Exception
+    {
+        AEADParameters parameters = new AEADParameters(new KeyParameter(new byte[keySize]), macSize * 8, new byte[ivSize], null);
+        int[] lags = new int[]{ -blockSize, 1 - blockSize, -1, 0, 1, blockSize };
+        for (int dataLen = 2; dataLen <= blockSize * 3 + 2; dataLen++)
+        {
+            byte[] data = new byte[dataLen];
+            for (int i = 0; i != dataLen; i++)
+            {
+                data[i] = (byte)i;
+            }
+            cipher.init(true, parameters);
+            byte[] expected = new byte[cipher.getOutputSize(dataLen)];
+            int len = cipher.processBytes(data, 0, dataLen, expected, 0);
+            cipher.doFinal(expected, len);
+
+            for (int i = 0; i != lags.length; i++)
+            {
+                for (int split = 1; split < data.length; split++)
+                {
+                    checkOverlappingSplit(test, true, parameters, data, expected, split, lags[i], blockSize + macSize, cipher);
+                }
+                for (int split = 1; split < expected.length; split++)
+                {
+                    checkOverlappingSplit(test, false, parameters, expected, data, split, lags[i], blockSize + macSize, cipher);
+                }
+            }
+        }
+    }
+
+    private static void checkOverlappingSplit(SimpleTest test, boolean forEncryption, AEADParameters parameters, byte[] input,
+        byte[] expected, int split, int lag, int margin, AEADCipher cipher)
+        throws Exception
+    {
+        if (lag > 0)
+        {
+            // with the output ahead of the input, a first call returning more than split - lag bytes would
+            // overwrite input the caller has not passed in yet, which is a caller error
+            cipher.init(forEncryption, parameters);
+            if (lag + cipher.processBytes(input, 0, split, new byte[input.length + margin], 0) > split)
+            {
+                return;
+            }
+        }
+        String label = (forEncryption ? "encryption" : "decryption") + " for " + cipher.getAlgorithmName()
+            + ", length " + input.length + " split " + split + " lag " + lag;
+        int inOff = margin;
+        int outOff = inOff + lag;
+        byte[] buf = new byte[inOff + input.length + 2 * margin];
+        System.arraycopy(input, 0, buf, inOff, input.length);
+        cipher.init(forEncryption, parameters);
+        int len = cipher.processBytes(buf, inOff, split, buf, outOff);
+        len += cipher.processBytes(buf, inOff + split, input.length - split, buf, outOff + len);
+        try
+        {
+            len += cipher.doFinal(buf, outOff + len);
+        }
+        catch (InvalidCipherTextException e)
+        {
+            test.fail("fail on testing split overlapping of " + label + ": " + e.getMessage());
+        }
+        test.isTrue("fail on testing split overlapping of " + label,
+            len == expected.length && Arrays.areEqual(expected, 0, expected.length, buf, outOff, outOff + len));
     }
 }

core/src/test/jdk1.3/org/bouncycastle/crypto/test/CipherTest.java

@@ -949,5 +949,78 @@ static void testOverlapping(SimpleTest test, int keySize, int ivSize, int macSiz
         test.isTrue("fail on testing overlapping of decryption for " + cipher.getAlgorithmName(),
             Arrays.areEqual(expected, 0, blockSize * 2, data, offset, offset + blockSize * 2));
 
+        testOverlappingSplit(test, keySize, ivSize, macSize, blockSize, cipher);
+    }
+
+    /**
+     * Encrypt and decrypt in two processBytes() calls with the input and the output in the same array, the
+     * output starting at, behind or ahead of the input, and compare with a single call into a separate array.
+     * The second call writes the bytes the first one buffered as well as its own, so its overlap check has to
+     * allow for them.
+     */
+    static void testOverlappingSplit(SimpleTest test, int keySize, int ivSize, int macSize, int blockSize, AEADCipher cipher)
+        throws Exception
+    {
+        AEADParameters parameters = new AEADParameters(new KeyParameter(new byte[keySize]), macSize * 8, new byte[ivSize], null);
+        int[] lags = new int[]{ -blockSize, 1 - blockSize, -1, 0, 1, blockSize };
+        for (int dataLen = 2; dataLen <= blockSize * 3 + 2; dataLen++)
+        {
+            byte[] data = new byte[dataLen];
+            for (int i = 0; i != dataLen; i++)
+            {
+                data[i] = (byte)i;
+            }
+            cipher.init(true, parameters);
+            byte[] expected = new byte[cipher.getOutputSize(dataLen)];
+            int len = cipher.processBytes(data, 0, dataLen, expected, 0);
+            cipher.doFinal(expected, len);
+
+            for (int i = 0; i != lags.length; i++)
+            {
+                for (int split = 1; split < data.length; split++)
+                {
+                    checkOverlappingSplit(test, true, parameters, data, expected, split, lags[i], blockSize + macSize, cipher);
+                }
+                for (int split = 1; split < expected.length; split++)
+                {
+                    checkOverlappingSplit(test, false, parameters, expected, data, split, lags[i], blockSize + macSize, cipher);
+                }
+            }
+        }
+    }
+
+    private static void checkOverlappingSplit(SimpleTest test, boolean forEncryption, AEADParameters parameters, byte[] input,
+        byte[] expected, int split, int lag, int margin, AEADCipher cipher)
+        throws Exception
+    {
+        if (lag > 0)
+        {
+            // with the output ahead of the input, a first call returning more than split - lag bytes would
+            // overwrite input the caller has not passed in yet, which is a caller error
+            cipher.init(forEncryption, parameters);
+            if (lag + cipher.processBytes(input, 0, split, new byte[input.length + margin], 0) > split)
+            {
+                return;
+            }
+        }
+        String label = (forEncryption ? "encryption" : "decryption") + " for " + cipher.getAlgorithmName()
+            + ", length " + input.length + " split " + split + " lag " + lag;
+        int inOff = margin;
+        int outOff = inOff + lag;
+        byte[] buf = new byte[inOff + input.length + 2 * margin];
+        System.arraycopy(input, 0, buf, inOff, input.length);
+        cipher.init(forEncryption, parameters);
+        int len = cipher.processBytes(buf, inOff, split, buf, outOff);
+        len += cipher.processBytes(buf, inOff + split, input.length - split, buf, outOff + len);
+        try
+        {
+            len += cipher.doFinal(buf, outOff + len);
+        }
+        catch (InvalidCipherTextException e)
+        {
+            test.fail("fail on testing split overlapping of " + label + ": " + e.getMessage());
+        }
+        test.isTrue("fail on testing split overlapping of " + label,
+            len == expected.length && Arrays.areEqual(expected, 0, expected.length, buf, outOff, outOff + len));
     }
 }

docs/releasenotes.md

@@ -63,6 +63,7 @@ Date: 2026, TBD
 - The ML-KEM KeyGenerator (KEMGenerateSpec/KEMExtractSpec), Cipher (wrap/unwrap), javax.crypto.KEM and KeyFactory.translateKey services accepted only BC's own ML-KEM key objects, and the KeyGenerator failed with a ClassCastException at generateKey() rather than at init, so an ML-KEM key from another provider could not be used with BC even though its standard encoding was one BC reads. This broke BCJSSE handshakes over the ML-KEM and hybrid groups whenever another provider ahead of BC decoded the peer's key or generated the ephemeral key pair. A foreign key is now converted from its X.509 or PKCS#8 encoding, with the usual parameter-set checks, and an unusable one is rejected at init (github #2466).
 - The raw JCA provider bounded the PBKDF2 iteration count taken from an encoding (org.bouncycastle.pbe.max_iteration_count, default 10,000,000) but not the counts of the legacy PBES1 (PKCS#5 scheme 1) and PKCS#12 PBE families beside it. Their AlgorithmParameters (PKCS12PBE and its OID aliases, PBKDF1) accepted any count, narrowing one beyond the int range with intValue() so that 2^32 arrived as 0, and every Cipher, Mac and SecretKeyFactory derivation ran with whatever count it was given - including a count decoded by another provider's AlgorithmParameters, as when javax.crypto.EncryptedPrivateKeyInfo.getKeySpec() decrypts a PKCS#12 PBE-protected key with BC. As these schemes carry the count in unauthenticated parameters and derive before anything can be checked, a supplied blob could hold a derivation for tens of minutes. The parameter parse now rejects a negative, beyond-int or over-limit count, and the derivations reject a negative or over-limit count, under the same property as PBKDF2. The PKCS#12 key store derives through the same code, so a org.bouncycastle.pkcs12.max_it_count raised above 10,000,000 now needs org.bouncycastle.pbe.max_iteration_count raised with it.
 - The light-weight CryptoProWrapEngine (RFC 4357 sec. 6.3) diversified the key encryption key in the caller's own array, so after init the KeyParameter it was given held the diversified key, and initialising again with the same parameters - to unwrap what had just been wrapped, say - diversified it a second time and used a different key. It also failed with a NullPointerException when given no S-box, although init has a branch for that case. It now diversifies a copy, and given no S-box uses the GOST 28147 engine's default S-box for the diversification, the one the wrap itself then uses. The provider's GOST 28147 key wrap ciphers were unaffected, as they always supply an S-box and build a new KeyParameter on every init.
+- AsconAEAD128 and most of the other lightweight AEAD engines built on AEADBaseEngine (AsconEngine, Elephant, GIFT-COFB, ISAP, PhotonBeetle, Romulus-N and Romulus-T, Sparkle and Xoodyak) could corrupt data encrypted or decrypted in place, with the input and the output in the same array, when the data was passed in over more than one call. Before writing anything, processBytes() copies its input aside if the output it is about to write overlaps that input, but it sized that output from the length passed in alone, while the call also writes out the bytes an earlier call left buffered, so an overlap could go unnoticed and the engine wrote output over input it had not yet read. Depending on the engine, the chunk sizes and where the output started relative to the input, a valid ciphertext failed its tag check, or the engine produced a wrong ciphertext, which the receiver either rejected or, in some cases, accepted, getting the wrong plaintext with no error raised: Romulus-N, for one, could emit a ciphertext with an all-zero block that still authenticated. The overlap check now counts the buffered bytes as well. Grain-128AEAD and Romulus-M take other paths through the base class and were unaffected.
 
 ### 2.1.3 Additional Features and Functionality