Skip to content

fix: heap-buffer-overflow in FlexBuffers ToString due to inverted VerifyKey predicate#9077

Open
SongTonyLi wants to merge 2 commits intogoogle:masterfrom
SongTonyLi:fix/issue-9041-verifykey-nul-terminator
Open

fix: heap-buffer-overflow in FlexBuffers ToString due to inverted VerifyKey predicate#9077
SongTonyLi wants to merge 2 commits intogoogle:masterfrom
SongTonyLi:fix/issue-9041-verifykey-nul-terminator

Conversation

@SongTonyLi
Copy link
Copy Markdown

Summary

Fixes #9041.

flexbuffers::VerifyBuffer accepts a 20-byte malformed FlexBuffer, but Reference::ToString then calls strlen on key data that extends past the buffer, causing a heap-buffer-overflow READ of size 5. The verifier's VerifyKey function has an inverted predicate: if (*p++) return true accepts any key containing a non-zero byte, without requiring a NUL terminator. Downstream consumers like Reference::ToString assume keys are NUL-terminated C strings and call strlen, reading past the buffer.

Root cause

Verifier::VerifyKey (flexbuffers.h:1976-1980) loops through key bytes and returns true on the first non-zero byte it sees:

while (p < buf_ + size_)
    if (*p++) return true;

This is backwards. It should accept when it finds a NUL terminator ('\0'), not when it finds a non-zero byte. The contract gap means malformed keys with no NUL terminator pass verification but break all downstream C-string operations.

I added trace probes to confirm:

  1. VerifyKey entered at offset 16, first byte 0x0d, buffer size 20
  2. VerifyKey accepted after scanning just 1 byte (the non-zero 0x0d), never seeing a NUL terminator
  3. ToString then called strlen on this unterminated key data and read 5 bytes past the 20-byte heap region

Fix

One-character change in VerifyKey: flip if (*p++) to if (!*p++). The function now returns true only when it finds an in-bounds NUL terminator, aligning the verifier contract with what all downstream C-string consumers expect.

Why this approach

I looked at two places:

  • Guard in Reference::ToString (crash site): would only protect this one output path. Malformed inputs would still pass VerifyBuffer and could break other key consumers (AsKey, map key printing, etc.). Didn't go with this.
  • Fix the predicate in VerifyKey (went with this): repairs the trust boundary so malformed keys are rejected before any consumer sees them. One-line fix, covers all downstream paths.

Verification

Tested with ASan-instrumented build (-fsanitize=address, detect_leaks=1):

  • Before: SUMMARY: AddressSanitizer: heap-buffer-overflow (READ of size 5 at flexbuffers.h:614 in ToString, exit=1)
  • After: VerifyBuffer failed (malformed PoC rejected at verifier, exit=2, no crash)

Regression testing

Ran the regression tester against the gold FlexBuffer sample (tests/gold_flexbuffer_example.bin):

Check Pre-patch Post-patch
Build exit=0 exit=0
PoC ASan run heap-buffer-overflow, exit=1 VerifyBuffer failed, exit=2
PoC gdb batch crash in ToString at flexbuffers.h:614 no crash, verifier rejection
Valid sample output { bar: [ 1, 2, 3 ], ... } exit=0 identical output, exit=0
Valgrind memcheck 0 errors 0 errors
cppcheck no security findings no new findings

Valid FlexBuffer parsing is completely unaffected. The only behavioral change is that malformed keys without NUL terminators are now correctly rejected by the verifier.

…ifyKey predicate

VerifyKey accepted keys on the first non-zero byte instead of requiring
a NUL terminator. Malformed keys passed verification, then ToString
called strlen on unterminated data, reading past the buffer.

Fix: flip the predicate from `if (*p++)` to `if (!*p++)` so VerifyKey
requires an in-bounds NUL terminator before accepting.

Fixes google#9041
@github-actions github-actions Bot added the c++ label May 6, 2026
@SongTonyLi SongTonyLi marked this pull request as ready for review May 6, 2026 17:09
@SongTonyLi SongTonyLi requested a review from dbaileychess as a code owner May 6, 2026 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Heap Buffer Overflow in FlexBuffers ToString Post-Verification

1 participant