fix: heap-buffer-overflow in FlexBuffers ToString due to inverted VerifyKey predicate#9077
Open
SongTonyLi wants to merge 2 commits intogoogle:masterfrom
Open
fix: heap-buffer-overflow in FlexBuffers ToString due to inverted VerifyKey predicate#9077SongTonyLi wants to merge 2 commits intogoogle:masterfrom
SongTonyLi wants to merge 2 commits intogoogle:masterfrom
Conversation
…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
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
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.
Summary
Fixes #9041.
flexbuffers::VerifyBufferaccepts a 20-byte malformed FlexBuffer, butReference::ToStringthen callsstrlenon key data that extends past the buffer, causing a heap-buffer-overflow READ of size 5. The verifier'sVerifyKeyfunction has an inverted predicate:if (*p++) return trueaccepts any key containing a non-zero byte, without requiring a NUL terminator. Downstream consumers likeReference::ToStringassume keys are NUL-terminated C strings and callstrlen, reading past the buffer.Root cause
Verifier::VerifyKey(flexbuffers.h:1976-1980) loops through key bytes and returnstrueon the first non-zero byte it sees: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:
VerifyKeyentered at offset 16, first byte0x0d, buffer size 20VerifyKeyaccepted after scanning just 1 byte (the non-zero0x0d), never seeing a NUL terminatorToStringthen calledstrlenon this unterminated key data and read 5 bytes past the 20-byte heap regionFix
One-character change in
VerifyKey: flipif (*p++)toif (!*p++). The function now returnstrueonly 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:
Reference::ToString(crash site): would only protect this one output path. Malformed inputs would still passVerifyBufferand could break other key consumers (AsKey, map key printing, etc.). Didn't go with this.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):SUMMARY: AddressSanitizer: heap-buffer-overflow(READ of size 5 atflexbuffers.h:614inToString, exit=1)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):VerifyBuffer failed, exit=2{ bar: [ 1, 2, 3 ], ... }exit=0Valid FlexBuffer parsing is completely unaffected. The only behavioral change is that malformed keys without NUL terminators are now correctly rejected by the verifier.