Fix missing null-terminator content check in FlexBuffers VerifyTerminator#9086
Open
Alearner12 wants to merge 1 commit into
Open
Fix missing null-terminator content check in FlexBuffers VerifyTerminator#9086Alearner12 wants to merge 1 commit into
Alearner12 wants to merge 1 commit into
Conversation
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.
Fixes #9085
Sibling fix to #9072 ("Fix logic inversion in FlexBuffers VerifyKey()", commit
a6979fe).Summary
flexbuffers::Verifier::VerifyTerminator()is intended to verify that anFBT_STRINGvalue has a valid null terminator at offsetsize, but the current implementation only verifies that the byte at that offset lies inside the buffer; it does not check that the byte equals0x00.VerifyFromPointer(p, size + 1)only confirms that the range[p, p + size + 1)lies withinbuf_. The terminator byte's value is never read.As a result,
VerifyBuffer()accepts FlexBuffers whose strings are not actually null-terminated. A subsequent call toAsString().c_str()followed bystrlen()/strcmp()/ any C-API call that expects a NUL-terminated string then reads past the verified region.Why this is the same bug class as #9072
#9072 fixed
VerifyKey(), which had the same shape: the verifier accepted a key whose terminator was missing, and downstreamstrlen()/strcmp()then ran past the buffer.VerifyTerminator()is the FBT_STRING equivalent helper, in the same class, called from the sameVerifyRef()switch (case FBT_STRING:).Compare to the non-flex
VerifyString()ininclude/flatbuffers/verifier.h, which gets it right by also checking the terminator byte's value:This PR aligns the FlexBuffers verifier with the non-flex one.
Fix
Test
Adds
FlexBuffersVerifyStringTerminatorTest()intests/flexbuffers_test.cpp. The test constructs a hand-rolled flexbuffer with anFBT_STRINGwhose terminator byte is'X'(non-zero) and assertsVerifyBuffer()returnsfalse. Without this PR, the same test fails (verifier returnstrue).I also confirmed under AddressSanitizer that, on the unpatched tree, the buffer is accepted by
VerifyBuffer()and a follow-upstrlen(root.AsString().c_str())triggers a heap-buffer-overflow read; with this PR the buffer is rejected and the OOB read does not occur.