Hi hackers,
ginPostingListDecodeAllSegments() sets endptr from segment->nbytes and
steps to the next segment with GinNextPostingListSegment(). Neither
checked against the end of the posting list, and decode_varbyte() has
no end pointer. So a corrupt page is read past its end, and its items
are decoded without being checked.
A corrupt page reaches this code through an index scan, VACUUM or WAL
replay. It is also directly reachable through pageinspect's
gin_leafpage_items(), which makes it convenient to reproduce with
crafted bytes as in the tests.
On a release build it returns invalid TIDs with no error. On an
assert build it aborts. I reproduced on 14 through 18 release
builds and on master.
I don't think this is a security issue. The normal paths require an
already-corrupt posting list, and the direct crafted-input path is
through pageinspect.
Seven patches:
0001 bound decode_varbyte() against the segment end
0002 check each segment fits within the posting list
0003 reject items that are out of range or out of order
0004 tests, through gin_leafpage_items()
0005 size the output array from len, not the unvalidated nbytes
0006 remove unreachable repalloc_array()
0007 scope some locals to the loop (cosmetic)
I split these deliberately as it made it easier for me to see what
each one fixes and understand the code along the way. Some may be
worth combining.
The change to decode_varbyte() replaces the seven nested if-blocks
with a loop. I tried adding the endptr checks in each block with a
shared goto label for the ereport(), but it was kind of unwieldy to
read through. There was no measurable difference between the two
approaches either.
0001 through 0003 also change what happens if a corrupt posting list
is encountered during WAL replay. The corruption now raises an error
and stops recovery instead of continuing after decoding garbage.
That seems preferable, but it is a behavioral change.
Sizing the result array from len (0005) should not over-allocate in
practice. A dense posting list runs about a byte per item, so len is
within a few percent of the item count, and the array is a transient
allocation bounded by the page anyway.
For backpatching, 0007 is cosmetic and master only. 0001 through 0005
are the fix and its test, all reachable back to 14, though the replay
behavior above is worth weighing before backpatching them.
Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/
