Skip to content

Reject LZ4 match offsets that point before the start of the output - #16478

Open
serhiy-bzhezytskyy wants to merge 2 commits into
apache:mainfrom
serhiy-bzhezytskyy:GITHUB-lz4-match-offset-bound
Open

Reject LZ4 match offsets that point before the start of the output#16478
serhiy-bzhezytskyy wants to merge 2 commits into
apache:mainfrom
serhiy-bzhezytskyy:GITHUB-lz4-match-offset-bound

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown

Description

LZ4#decompress reads a match offset and checks only that it is not zero. It does not check it against the number of bytes written so far, so an offset larger than dOff makes the match reference bytes before dest[0] — bytes this call never wrote:

final int matchDec = compressed.readShort() & 0xFFFF;
if (matchDec == 0) {
  throw new IOException("offset 0 is invalid");
}
...
for (int ref = dOff - matchDec, end = dOff + matchLen; dOff < end; ++ref, ++dOff) {
  dest[dOff] = dest[ref];          // ref can be negative
}
...
System.arraycopy(dest, dOff - matchDec, dest, dOff, fastLen);   // same

Corrupt input therefore fails with ArrayIndexOutOfBoundsException: arraycopy: source index -8 out of bounds for byte[18] rather than a checked IOException.

The LZ4 block format leaves this to the implementation, and @jpountz described the same condition on LUCENE-5267 in 2013 while diagnosing an AIOOBE from a corrupt .fdt:

all the lines you pasted make no sense since matchDec should be lower than dOff

The compressor already asserts the invariant when writing a match — assert matchDec > 0 && matchDec < 1 << 16 at LZ4.java:188 — so this only affects input that was not produced by LZ4#compress, which is to say corrupt or hostile input.

What this does and does not fix

It makes the failure legible. It does not improve corruption detection, and it is worth being explicit about that, because the two are easy to conflate.

Measured on main, flipping one byte at a time across the .fdt of a 2,000-document index (codec and mode pinned to Lucene104/BEST_SPEED, every 37th byte, all documents read back and compared):

outcome main with this change
ArrayIndexOutOfBoundsException from LZ4 36.1% 0%
IOException 0.3% 35.8%
incorrect stored-field value returned, no error at all 48.5% 47.7%
CorruptIndexException 0.8% 0.6%
NullPointerException (header or index corruption) 8.8% 9.1%

So the AIOOBE becomes a checked exception almost one-for-one, and the largest outcome — a wrong document returned silently — is untouched. A validity check cannot detect a valid-but-wrong LZ4 stream; most flipped bytes still decode to something, just not to what was compressed. Only a checksum can catch that, which is what LUCENE-5267 proposed and what the frame format specifies as an optional per-block checksum.

That sampling is coarse: 2.7% of positions, all eight bits inverted rather than one, only the first exception classified, one corpus and one compression mode. It is offered as the shape of the change, not as a precise figure.

Relationship to #15570

matchDec == 0 was closed there in January. This is the other unchecked path in the same method. lucene-core is not affected by CVE-2025-66566 — that is a lz4-java advisory, and Lucene has its own vendored LZ4 — but the advisory describes the general shape as "an attacker can direct such a copy at a region that hasn't been filled with decompressed bytes yet", and cites it as prior art for why the bound matters. Whether Lucene's stored-fields path reuses output buffers such that another document's bytes could surface is not something I measured, so this is proposed as format conformance and error legibility, not as a security fix.

Verification

  • Both new tests fail without the change, with AIOOBE: source index -8 and source index -1
  • Removing the check and re-running (mutation check) fails; restoring it passes
  • The dictionary case is covered separately: with a preset dictionary the bound is dOff rather than the number of bytes this call decompressed, since a match may legitimately reference the dictionary the caller placed in dest[dOff-dictLen:dOff]
  • :lucene:core:test for *LZ4*, *Compressing* and *StoredFields* passes with -Ptests.nightly=true -Ptests.iters=5 (1,375 tests)
  • :lucene:core:check and tidy pass

The CHANGES.txt entry uses GITHUB#PENDING; I will replace it with this PR's number.

LZ4#decompress read a match offset without checking it against the number
of bytes written so far. An offset larger than dOff makes the match
reference bytes before dest[0], which this call never wrote, so corrupt
input failed with ArrayIndexOutOfBoundsException from System.arraycopy or
from the incremental copy loop rather than with a checked IOException.

The bound is dOff and not the number of bytes decompressed by this call,
because a preset dictionary is placed in dest[dOff-dictLen:dOff] and a
match may legitimately reference it.

The compressor already asserts the same invariant when writing a match
(matchDec > 0 && matchDec < 1 << 16 at LZ4.java:183), so this only
affects input that was not produced by LZ4#compress.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant