From e70aabea36353867eb7a12c9aebf97c3de10c136 Mon Sep 17 00:00:00 2001 From: Antoine Gruzelle Date: Mon, 11 May 2026 14:21:48 +0200 Subject: [PATCH 1/2] Show and fix the invisible flaw of DecryptingIndexInput 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. --- .../crypto/DecryptingIndexInput.java | 2 +- .../crypto/DecryptingIndexInputTest.java | 37 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/encryption/src/main/java/org/apache/solr/encryption/crypto/DecryptingIndexInput.java b/encryption/src/main/java/org/apache/solr/encryption/crypto/DecryptingIndexInput.java index a459330..0345428 100644 --- a/encryption/src/main/java/org/apache/solr/encryption/crypto/DecryptingIndexInput.java +++ b/encryption/src/main/java/org/apache/solr/encryption/crypto/DecryptingIndexInput.java @@ -164,7 +164,7 @@ public void seek(long position) throws IOException { long targetPosition = position + sliceOffset; long delegatePosition = indexInput.getFilePointer(); long currentPosition = delegatePosition - (outSize - outPos); - if (targetPosition >= currentPosition && targetPosition <= delegatePosition) { + if (outSize > 0 && targetPosition >= currentPosition && targetPosition <= delegatePosition) { // The target position is within the buffered output. Just move the output buffer position. outPos += (int) (targetPosition - currentPosition); assert targetPosition == delegatePosition - (outSize - outPos); diff --git a/encryption/src/test/java/org/apache/solr/encryption/crypto/DecryptingIndexInputTest.java b/encryption/src/test/java/org/apache/solr/encryption/crypto/DecryptingIndexInputTest.java index 0e6337d..e820cf3 100644 --- a/encryption/src/test/java/org/apache/solr/encryption/crypto/DecryptingIndexInputTest.java +++ b/encryption/src/test/java/org/apache/solr/encryption/crypto/DecryptingIndexInputTest.java @@ -151,6 +151,43 @@ public void testSeekEmpty() throws Exception { indexInput.close(); } + /** + * Reproduces a bug where creating a sub-slice at offset 0 from a slice at a non-zero offset + * would produce corrupted decrypted data. This is the pattern used by + * {@link IndexInput#randomAccessSlice(long, long)}, which internally calls + * slice("randomaccess", 0, length). + * + * The root cause: when slice() creates a new DecryptingIndexInput, the + * constructor sets encrypter.init(0) (AES counter = 0) and the subsequent + * seek(0) is supposed to call setPosition() to initialize the correct + * AES counter and padding. But when the cloned delegate's file pointer already equals the + * target position (which happens with offset=0), seek() took a buffer shortcut + * and skipped setPosition(), leaving the counter and padding at wrong values. + * + * With prefixSize=17: counter should be 17/16=1 (not 0) and padding should be 17%16=1 (not 0). + * Both are wrong without the fix, triggering both dimensions of the bug. + */ + @Test + public void testRandomAccessSlice() throws Exception { + ByteBuffersDataOutput dataOutput = new ByteBuffersDataOutput(); + EncryptingIndexOutput indexOutput = createEncryptingIndexOutput(dataOutput); + int bytesBeforeFirstSlice = 17; + byte[] prefix = new byte[bytesBeforeFirstSlice]; + indexOutput.writeBytes(prefix, 0, prefix.length); + String data = "Hello, world!"; + indexOutput.writeBytes(data.getBytes(), 0, data.length()); + indexOutput.close(); + + DecryptingIndexInput root = createDecryptingIndexInput(dataOutput, 0); + // Simulates reading a sub-file (.tip for example) at a non-zero offset inside a compound file (.cfs) + IndexInput firstSlice = root.slice(".tip", bytesBeforeFirstSlice, data.length()); + // Simulates randomAccessSlice(0, length) - creates a sub-slice at offset 0 + IndexInput subSlice = firstSlice.slice("randomaccess", 0, data.length()); + byte[] result = new byte[data.length()]; + subSlice.readBytes(result, 0, data.length()); + assertEquals(data, new String(result)); + } + @Test public void testSeek() throws Exception { for (int reps = randomIntBetween(1, 200); --reps > 0; ) { From 317908c96d2b2c0cd1e521d686d94ad7ad4bb6b9 Mon Sep 17 00:00:00 2001 From: Antoine Gruzelle Date: Mon, 11 May 2026 14:28:55 +0200 Subject: [PATCH 2/2] Extract DecryptionBuffer into private class 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. --- .../crypto/DecryptingIndexInput.java | 206 ++++++++++++------ 1 file changed, 136 insertions(+), 70 deletions(-) diff --git a/encryption/src/main/java/org/apache/solr/encryption/crypto/DecryptingIndexInput.java b/encryption/src/main/java/org/apache/solr/encryption/crypto/DecryptingIndexInput.java index 0345428..da1f3fa 100644 --- a/encryption/src/main/java/org/apache/solr/encryption/crypto/DecryptingIndexInput.java +++ b/encryption/src/main/java/org/apache/solr/encryption/crypto/DecryptingIndexInput.java @@ -51,14 +51,8 @@ public class DecryptingIndexInput extends FilterIndexInput { private final long sliceOffset; private final long sliceEnd; private IndexInput indexInput; - private AesCtrEncrypter encrypter; - private byte[] inBuffer; - private byte[] outBuffer; + private DecryptionBuffer buffer; private byte[] oneByteBuf; - private int inPos; - private int outPos; - private int outSize; - private int padding; private boolean closed; /** @@ -111,11 +105,7 @@ private DecryptingIndexInput(String resourceDescription, this.sliceEnd = sliceOffset + sliceLength; this.isClone = isClone; this.indexInput = indexInput; - this.encrypter = encrypter; - encrypter.init(0); - assert bufferCapacity % AES_BLOCK_SIZE == 0; - inBuffer = new byte[bufferCapacity]; - outBuffer = new byte[bufferCapacity + AES_BLOCK_SIZE]; + this.buffer = new DecryptionBuffer(encrypter, bufferCapacity); oneByteBuf = new byte[1]; } @@ -150,7 +140,7 @@ public long getFilePointer() { * Gets the current internal position in the delegate {@link IndexInput}. It includes IV length. */ private long getPosition() { - return indexInput.getFilePointer() - (outSize - outPos); + return indexInput.getFilePointer() - buffer.bufferedAhead(); } @Override @@ -163,27 +153,17 @@ public void seek(long position) throws IOException { } long targetPosition = position + sliceOffset; long delegatePosition = indexInput.getFilePointer(); - long currentPosition = delegatePosition - (outSize - outPos); - if (outSize > 0 && targetPosition >= currentPosition && targetPosition <= delegatePosition) { + long currentPosition = delegatePosition - buffer.bufferedAhead(); + if (buffer.hasData() && targetPosition >= currentPosition && targetPosition <= delegatePosition) { // The target position is within the buffered output. Just move the output buffer position. - outPos += (int) (targetPosition - currentPosition); - assert targetPosition == delegatePosition - (outSize - outPos); + buffer.skipBytes((int) (targetPosition - currentPosition)); + assert targetPosition == delegatePosition - buffer.bufferedAhead(); } else { indexInput.seek(targetPosition); - setPosition(targetPosition); + buffer.seek(targetPosition, delegateOffset); } } - private void setPosition(long position) { - outPos = outSize = 0; - // Compute the counter by ignoring the IV and the delegate offset, if any. - long delegatePosition = position - delegateOffset; - long counter = delegatePosition / AES_BLOCK_SIZE; - encrypter.init(counter); - padding = (int) (delegatePosition & AES_BLOCK_SIZE_MOD_MASK); - inPos = padding; - } - /** * Returns the number of encrypted/decrypted bytes in the file. *

It is the logical length of the file, not the physical length. It excludes the IV added artificially to manage @@ -203,7 +183,7 @@ public IndexInput slice(String sliceDescription, long offset, long length) throw } DecryptingIndexInput slice = new DecryptingIndexInput( getFullSliceDescription(sliceDescription), delegateOffset, sliceOffset + offset, length, true, - indexInput.clone(), encrypter.clone(), inBuffer.length); + indexInput.clone(), buffer.cloneEncrypter(), buffer.capacity()); slice.seek(0); return slice; } @@ -224,42 +204,7 @@ public void readBytes(byte[] b, int offset, int length) throws IOException { throw new EOFException("Read beyond EOF (position=" + (getPosition() - sliceOffset) + ", arrayLength=" + length + ", fileLength=" + length() + ") in " + this); } - while (length > 0) { - // Transfer decrypted bytes from outBuffer. - int outRemaining = outSize - outPos; - if (outRemaining > 0) { - if (length <= outRemaining) { - System.arraycopy(outBuffer, outPos, b, offset, length); - outPos += length; - return; - } - System.arraycopy(outBuffer, outPos, b, offset, outRemaining); - outPos += outRemaining; - offset += outRemaining; - length -= outRemaining; - } - readToFillBuffer(length); - decryptBuffer(); - } - } - - private void readToFillBuffer(int length) throws IOException { - assert length > 0; - int inRemaining = inBuffer.length - inPos; - if (inRemaining > 0) { - int numBytesToRead = Math.min(inRemaining, length); - indexInput.readBytes(inBuffer, inPos, numBytesToRead); - inPos += numBytesToRead; - } - } - - private void decryptBuffer() { - assert inPos > padding : "inPos=" + inPos + " padding=" + padding; - encrypter.process(inBuffer, 0, inPos, outBuffer, 0); - outSize = inPos; - inPos = 0; - outPos = padding; - padding = 0; + buffer.readDecrypted(indexInput, b, offset, length); } @Override @@ -268,12 +213,133 @@ public DecryptingIndexInput clone() { clone.isClone = true; clone.indexInput = indexInput.clone(); assert clone.indexInput.getFilePointer() == indexInput.getFilePointer(); - clone.encrypter = encrypter.clone(); - clone.inBuffer = new byte[inBuffer.length]; - clone.outBuffer = new byte[outBuffer.length]; + clone.buffer = buffer.clone(); clone.oneByteBuf = new byte[1]; // The clone must be initialized. - clone.setPosition(getPosition()); + clone.buffer.seek(getPosition(), delegateOffset); return clone; } -} \ No newline at end of file + + /** + * Manages the AES-CTR decryption buffers and encrypter state. + *

Reads encrypted bytes from a delegate {@link IndexInput} into an input buffer, + * decrypts them into an output buffer, and serves decrypted bytes to the caller. + */ + private static class DecryptionBuffer implements Cloneable { + + AesCtrEncrypter encrypter; + byte[] inBuffer; + byte[] outBuffer; + int inPos; + int outPos; + int outSize; + int padding; + + DecryptionBuffer(AesCtrEncrypter encrypter, int bufferCapacity) { + assert bufferCapacity % AES_BLOCK_SIZE == 0; + this.encrypter = encrypter; + encrypter.init(0); + inBuffer = new byte[bufferCapacity]; + outBuffer = new byte[bufferCapacity + AES_BLOCK_SIZE]; + } + + /** Whether there are decrypted bytes available in the output buffer. */ + boolean hasData() { + return outSize > 0; + } + + /** Number of decrypted bytes in the output buffer not yet consumed. */ + int bufferedAhead() { + return outSize - outPos; + } + + /** The input buffer capacity (used when creating slices with the same buffer size). */ + int capacity() { + return inBuffer.length; + } + + /** + * Advances the read position within the already-decrypted output buffer. + * The caller must ensure the target falls within the buffered range. + */ + void skipBytes(int bytesCount) { + outPos += bytesCount; + assert outPos <= outSize; + } + + /** + * Resets the buffer state and initializes the AES-CTR counter and padding for the given + * absolute position in the delegate file. + * + * @param absolutePosition position in the delegate {@link IndexInput} (includes IV offset) + * @param delegateOffset the position right after the IV — the "zero" of the AES-CTR stream + */ + void seek(long absolutePosition, long delegateOffset) { + outPos = outSize = 0; + long relativePosition = absolutePosition - delegateOffset; + long counter = relativePosition / AES_BLOCK_SIZE; + encrypter.init(counter); + padding = (int) (relativePosition & AES_BLOCK_SIZE_MOD_MASK); + inPos = padding; + } + + /** + * Reads and decrypts bytes from the delegate into the target array. + * Serves from the output buffer first, then reads and decrypts more as needed. + */ + void readDecrypted(IndexInput delegate, byte[] target, int offset, int length) throws IOException { + while (length > 0) { + int outRemaining = outSize - outPos; + if (outRemaining > 0) { + if (length <= outRemaining) { + System.arraycopy(outBuffer, outPos, target, offset, length); + outPos += length; + return; + } + System.arraycopy(outBuffer, outPos, target, offset, outRemaining); + outPos += outRemaining; + offset += outRemaining; + length -= outRemaining; + } + readFromDelegate(delegate, length); + decrypt(); + } + } + + private void readFromDelegate(IndexInput delegate, int length) throws IOException { + assert length > 0; + int inRemaining = inBuffer.length - inPos; + if (inRemaining > 0) { + int numBytesToRead = Math.min(inRemaining, length); + delegate.readBytes(inBuffer, inPos, numBytesToRead); + inPos += numBytesToRead; + } + } + + private void decrypt() { + assert inPos > padding : "inPos=" + inPos + " padding=" + padding; + encrypter.process(inBuffer, 0, inPos, outBuffer, 0); + outSize = inPos; + inPos = 0; + outPos = padding; + padding = 0; + } + + AesCtrEncrypter cloneEncrypter() { + return encrypter.clone(); + } + + @Override + public DecryptionBuffer clone() { + try { + DecryptionBuffer clone = (DecryptionBuffer) super.clone(); + clone.encrypter = encrypter.clone(); + clone.inBuffer = new byte[inBuffer.length]; + clone.outBuffer = new byte[outBuffer.length]; + return clone; + } catch (CloneNotSupportedException e) { + throw new AssertionError(e); + } + } + } +}