Fix invisible flaw in DecryptingIndexInput.seek() and extract DecryptionBuffer#131
Open
antogruz wants to merge 2 commits into
Open
Fix invisible flaw in DecryptingIndexInput.seek() and extract DecryptionBuffer#131antogruz wants to merge 2 commits into
antogruz wants to merge 2 commits into
Conversation
When slice() creates a new DecryptingIndexInput, the constructor leaves
the AES-CTR counter at 0 (encrypter.init(0)) and relies on the
subsequent seek(0) call to invoke setPosition() with the correct
counter and padding for the slice's actual offset.
However, when the cloned delegate's file pointer already equals the
target position - which happens when the slice is created at offset 0 -
seek() takes a buffered-output shortcut and skips setPosition()
altogether. The shortcut relies on outSize/outPos to express the
buffered range, but they are still 0 on a fresh DecryptingIndexInput,
so the check (targetPosition >= currentPosition &&
targetPosition <= delegatePosition) trivially succeeds with an empty
buffer. The encrypter is then left initialised at counter=0 with
padding=0, producing corrupted decrypted bytes for any read that
follows.
The fix is to guard the shortcut with outSize > 0 so that a fresh
slice always goes through setPosition() on its seek(0).
This bug is exercised by IndexInput#randomAccessSlice(long, long),
which internally calls slice("randomaccess", 0, length): when invoked
on a slice that itself starts at a non-zero offset (e.g. a sub-file of
a compound file), the nested slice silently returned wrong data.
Adds a regression test (testRandomAccessSlice) with prefixSize=17 so
both the AES counter (17/16=1) and the padding (17%16=1) are non-zero,
ensuring both dimensions of the bug are caught. The test fails on the
previous code with a ComparisonFailure and passes with the fix.
Move all the AES-CTR buffering state (encrypter, in/out byte buffers, their positions and sizes, and the leading padding) and the methods that operate on it (readToFillBuffer, decryptBuffer, the previous setPosition()) into a new private static inner class DecryptionBuffer. After this refactoring, DecryptingIndexInput keeps only the IndexInput-level concerns (file pointer, slice bounds, clone/close lifecycle) and delegates every byte-level encryption operation to the buffer: - getPosition() -> buffer.bufferedAhead() - seek() -> buffer.hasData() / skipBytes() / seek() - slice() -> buffer.cloneEncrypter() / capacity() - readBytes() -> buffer.readDecrypted() - clone() -> buffer.clone() / seek() The behavior is unchanged - in particular the seek() guard introduced in the previous commit is preserved, now expressed as buffer.hasData() instead of outSize > 0. The seven existing DecryptingIndexInputTest tests still pass. This makes the buffering state self-contained (easier to reason about counter and padding initialization) and shrinks DecryptingIndexInput's own surface, which should make future evolutions of either layer easier to review.
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
1 participant
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.
This PR contains two commits, well separated:
1.
Show and fix the invisible flaw of DecryptingIndexInputA one-line fix in
seek()plus a regression test.When
slice()produces a freshDecryptingIndexInput, the constructor leaves the AES-CTR encrypter at counter 0 and relies on the immediateseek(0)to callsetPosition()with the correct counter and padding for the slice's actual offset. But when the cloned delegate's file pointer already matches the target position - which happens whenever the slice is created at offset 0 -seek()was taking a buffered-output shortcut and skippingsetPosition()entirely. With an empty buffer (outSize == 0on a fresh slice), the shortcut conditiontargetPosition >= currentPosition && targetPosition <= delegatePositiontrivially holds, leaving the encrypter at counter=0 with padding=0. Any subsequent read returned corrupted plaintext.The fix is to guard the shortcut with
outSize > 0, so that a fresh slice always goes throughsetPosition()on itsseek(0).The bug is exercised by
IndexInput#randomAccessSlice(long, long), which internally callsslice("randomaccess", 0, length). A new testtestRandomAccessSlicereproduces it withprefixSize=17so both the AES counter (17/16=1) and the padding (17%16=1) are non-zero, ensuring both dimensions of the bug are caught.2.
Extract DecryptionBuffer into private classPure refactoring. Moves all the AES-CTR buffering state (encrypter, in/out byte buffers, positions, padding) and the methods that operate on it into a new private static inner class
DecryptionBuffer.DecryptingIndexInputkeeps only theIndexInput-level concerns (file pointer, slice bounds, clone/close lifecycle) and delegates byte-level operations to the buffer.No behavior change. The seven existing
DecryptingIndexInputTesttests still pass, including the guard introduced by commit 1, which is now expressed asbuffer.hasData()instead ofoutSize > 0.