AEADBaseEngine.processEncDecBytes (core/src/main/java/org/bouncycastle/crypto/engines/AEADBaseEngine.java:1053) copies its input aside when input == output and the output overlaps it (:1069), but sizes that output as length from processor.getUpdateOutputSize(len) (:1065), the current call's bytes only. The call also writes out the m_bufPos bytes an earlier call left in m_buf (:1082 when encrypting, :1101 and :1118 when decrypting), already counted at :1066 for the output check. When those bytes carry the write into the input, the check reports no overlap and input taken straight from the caller's array (:1089, :1123) or copied into m_buf (:1110, :1115, :1129) is read after output has overwritten it.
Reproduced on released 1.86 and the 1.87-SNAPSHOT beta (1.87.0.20730); main has the same source, compiling to the beta's bytecode. Romulus-N, 4113 bytes in 4096-byte chunks in one array, output offset at the bytes output so far: the second call is processBytes(buf, 4096, 17, buf, 4080) with 16 bytes buffered, the check covers only [4080, 4096) while the write spans [4080, 4112), ciphertext block 256 comes out all zero, and a separate-buffer receiver accepts it, getting the wrong plaintext. Decrypting the valid ciphertext the same way throws InvalidCipherTextException "Romulus-N mac does not match". AsconAEAD128 encrypting 9 then 8 bytes, output one byte ahead, likewise authenticates with the last plaintext byte wrong.
21 of the 23 parameter sets that call CipherTest.testOverlapping are affected at some offset. Grain-128AEAD (own check at :742) and Romulus-M are not. GCMBlockCipher (:395) and ChaCha20Poly1305 (:334) pass getUpdateOutputSize(len), which counts buffered bytes; this change uses the loop's own fields, as ElephantEngine and Grain128AEADEngine override that method.
This change:
passes length + m_bufPos to Arrays.segmentsOverlap at :1069, as :1066 does before subtracting MAC_SIZE on decryption (an overstated span costs at most a copy); and
adds testOverlappingSplit to CipherTest, called at the end of testOverlapping (CipherTest.java:945). It encrypts and decrypts in place in two processBytes calls, at every split, output at -b, 1-b, -1, 0, 1 and b bytes from the input (b the blockSize passed to testOverlapping), and compares with one call into a separate array. Positive offsets are skipped where the first call's output would reach input not yet passed in. The jdk1.3 copy gets the same addition.
Without the change 21 of the 23 fail the new test on the beta; with it all 23 pass. Across RegressionTest.tests only the eight affected LWC tests change status. With no bc-test-data here, their KAT files were not run. :core:checkstyleMain is clean.
Base tree only for the engine: one AEADBaseEngine.java, no META-INF/versions copy, no module-info or OSGi change. A release-note entry is included, happy to move it to another block.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AEADBaseEngine.processEncDecBytes (core/src/main/java/org/bouncycastle/crypto/engines/AEADBaseEngine.java:1053) copies its input aside when input == output and the output overlaps it (:1069), but sizes that output as length from processor.getUpdateOutputSize(len) (:1065), the current call's bytes only. The call also writes out the m_bufPos bytes an earlier call left in m_buf (:1082 when encrypting, :1101 and :1118 when decrypting), already counted at :1066 for the output check. When those bytes carry the write into the input, the check reports no overlap and input taken straight from the caller's array (:1089, :1123) or copied into m_buf (:1110, :1115, :1129) is read after output has overwritten it.
Reproduced on released 1.86 and the 1.87-SNAPSHOT beta (1.87.0.20730); main has the same source, compiling to the beta's bytecode. Romulus-N, 4113 bytes in 4096-byte chunks in one array, output offset at the bytes output so far: the second call is processBytes(buf, 4096, 17, buf, 4080) with 16 bytes buffered, the check covers only [4080, 4096) while the write spans [4080, 4112), ciphertext block 256 comes out all zero, and a separate-buffer receiver accepts it, getting the wrong plaintext. Decrypting the valid ciphertext the same way throws InvalidCipherTextException "Romulus-N mac does not match". AsconAEAD128 encrypting 9 then 8 bytes, output one byte ahead, likewise authenticates with the last plaintext byte wrong.
21 of the 23 parameter sets that call CipherTest.testOverlapping are affected at some offset. Grain-128AEAD (own check at :742) and Romulus-M are not. GCMBlockCipher (:395) and ChaCha20Poly1305 (:334) pass getUpdateOutputSize(len), which counts buffered bytes; this change uses the loop's own fields, as ElephantEngine and Grain128AEADEngine override that method.
This change:
Without the change 21 of the 23 fail the new test on the beta; with it all 23 pass. Across RegressionTest.tests only the eight affected LWC tests change status. With no bc-test-data here, their KAT files were not run. :core:checkstyleMain is clean.
Base tree only for the engine: one AEADBaseEngine.java, no META-INF/versions copy, no module-info or OSGi change. A release-note entry is included, happy to move it to another block.