Conversation
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 wheninput == outputand the output overlaps it (:1069), but sizes that output aslengthfromprocessor.getUpdateOutputSize(len)(:1065), the current call's bytes only. The call also writes out them_bufPosbytes an earlier call left inm_buf(:1082when encrypting,:1101and:1118when decrypting), already counted at:1066for 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 intom_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);
mainhas 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 isprocessBytes(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 throwsInvalidCipherTextException"Romulus-N mac does not match".AsconAEAD128encrypting 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.testOverlappingare affected at some offset. Grain-128AEAD (own check at:742) and Romulus-M are not.GCMBlockCipher(:395) andChaCha20Poly1305(:334) passgetUpdateOutputSize(len), which counts buffered bytes; this change uses the loop's own fields, asElephantEngineandGrain128AEADEngineoverride that method.This change:
length + m_bufPostoArrays.segmentsOverlapat:1069, as:1066does before subtractingMAC_SIZEon decryption (an overstated span costs at most a copy); andtestOverlappingSplittoCipherTest, called at the end oftestOverlapping(CipherTest.java:945). It encrypts and decrypts in place in twoprocessBytescalls, at every split, output at -b, 1-b, -1, 0, 1 and b bytes from the input (b theblockSizepassed totestOverlapping), 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.testsonly the eight affected LWC tests change status. With no bc-test-data here, their KAT files were not run.:core:checkstyleMainis clean.Base tree only for the engine: one
AEADBaseEngine.java, noMETA-INF/versionscopy, nomodule-infoor OSGi change. A release-note entry is included, happy to move it to another block.