FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Count buffered bytes in the AEADBaseEngine in-place input/output overlap check by Arpan0995 · Pull Request #2487 · bcgit/bc-java · GitHub

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

Open
Arpan0995 wants to merge 1 commit into
bcgit:mainfrom
Arpan0995:aeadbaseengine-inplace-buffered-overlap
Open

Arpan0995 wants to merge 1 commit into
bcgit:mainfrom
Arpan0995:aeadbaseengine-inplace-buffered-overlap

Conversation

Copy link
Copy Markdown
Contributor

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant


Back | FazBrowse Home | New Git URL