From 3f34c39ded57945002affb16bbf6e0e451862fec Mon Sep 17 00:00:00 2001 From: AsroyxCySec Date: Sun, 26 Jul 2026 08:33:46 +0700 Subject: [PATCH] FlexBuffers: reject cyclic buffers in the C++ Verifier The FlexBuffers verifier accepted cyclic buffers when a reuse_tracker was supplied. Because offsets point backwards, a vector/map element can reference an ancestor node that is still being verified; the reuse tracker's de-duplication treated that re-visit as "already verified" and returned true, so verification passed. Recursive accessors (Reference::ToString() and any user traversal) then followed the cycle with no depth/cycle guard, recursing until the stack was exhausted (DoS). Without a reuse_tracker the depth limit already rejected these buffers; the tracker's early return is what bypassed it. Make the reuse tracking cycle-aware in VerifyVector: mark a node in-progress while its children are verified and mark it verified on completion. A re-visit of an in-progress node is a cycle (reject); a re-visit of a completed node with the same type is a legitimate shared/DAG reference (skip, as before). This preserves DAG de-duplication of shared keys/strings and keeps verification linear. Also add a regression test (FlexBuffersCyclicBufferTest) and extend flexbuffers_verifier_fuzzer to access the buffer after a successful verify, so the "verified => safe to access" property is fuzzed going forward. --- include/flatbuffers/flexbuffers.h | 22 +++++++++++++++++++-- tests/flexbuffers_test.cpp | 15 ++++++++++++++ tests/flexbuffers_test.h | 1 + tests/fuzzer/flexbuffers_verifier_fuzzer.cc | 9 ++++++++- tests/test.cpp | 1 + 5 files changed, 45 insertions(+), 3 deletions(-) diff --git a/include/flatbuffers/flexbuffers.h b/include/flatbuffers/flexbuffers.h index 5c42a7ed4..d029c0eb2 100644 --- a/include/flatbuffers/flexbuffers.h +++ b/include/flatbuffers/flexbuffers.h @@ -1930,8 +1930,25 @@ class Verifier FLATBUFFERS_FINAL_CLASS { return false; auto size_byte_width = r.byte_width_; if (!VerifyBeforePointer(p, size_byte_width)) return false; - FLEX_CHECK_VERIFIED(p - size_byte_width, - PackedType(Builder::WidthB(size_byte_width), r.type_)); + // Cycle-aware reuse tracking. A node that is still being verified (i.e. + // present on the current verification path) is a cycle and must be + // rejected; a fully-verified node with the same type is a legitimate + // shared (DAG) reference and can be skipped. Distinguishing the two + // prevents cyclic buffers from passing verification -- which otherwise + // caused unbounded recursion / stack exhaustion in accessors such as + // Reference::ToString() -- while preserving the DAG de-duplication the + // reuse tracker exists for. + const uint8_t kInProgressMarker = 0xFF; // not a valid PackedType value. + const auto vpacked = PackedType(Builder::WidthB(size_byte_width), r.type_); + const size_t vpos = static_cast((p - size_byte_width) - buf_); + if (reuse_tracker_) { + FLATBUFFERS_ASSERT(vpos < reuse_tracker_->size()); + auto existing = (*reuse_tracker_)[vpos]; + if (existing == kInProgressMarker) return false; // cycle detected. + if (existing == vpacked) return true; // shared (DAG) node. + if (!Check(existing == 0)) return false; // type mismatch. + (*reuse_tracker_)[vpos] = kInProgressMarker; + } auto sized = Sized(p, size_byte_width); auto num_elems = sized.size(); auto elem_byte_width = r.type_ == FBT_STRING || r.type_ == FBT_BLOB @@ -1955,6 +1972,7 @@ class Verifier FLATBUFFERS_FINAL_CLASS { } else { FLATBUFFERS_ASSERT(IsInline(elem_type)); } + if (reuse_tracker_) (*reuse_tracker_)[vpos] = vpacked; // fully verified. depth_--; return true; } diff --git a/tests/flexbuffers_test.cpp b/tests/flexbuffers_test.cpp index 6087a0aff..5fb804a61 100644 --- a/tests/flexbuffers_test.cpp +++ b/tests/flexbuffers_test.cpp @@ -187,6 +187,21 @@ void FlexBuffersReuseBugTest() { true); } +void FlexBuffersCyclicBufferTest() { + // Cyclic buffers must be rejected by the verifier. Before the cycle-aware + // reuse-tracking fix, these buffers passed VerifyBuffer() when a reuse + // tracker was supplied, and then caused unbounded recursion / stack + // exhaustion in recursive accessors such as Reference::ToString(). + // A vector whose only element is the vector itself (self-loop). + const uint8_t self_loop[] = { 0x01, 0x00, 0x28, 0x01 }; + // A vector element that points back to the parent vector (parent cycle). + const uint8_t parent_cycle[] = { 0x02, 0x24, 0x01, 0x02, 0x28, 0x01 }; + std::vector t1, t2; + TEST_EQ(flexbuffers::VerifyBuffer(self_loop, sizeof(self_loop), &t1), false); + TEST_EQ(flexbuffers::VerifyBuffer(parent_cycle, sizeof(parent_cycle), &t2), + false); +} + void FlexBuffersFloatingPointTest() { #if defined(FLATBUFFERS_HAS_NEW_STRTOD) && (FLATBUFFERS_HAS_NEW_STRTOD > 0) flexbuffers::Builder slb(512, diff --git a/tests/flexbuffers_test.h b/tests/flexbuffers_test.h index 132098fb3..bc6e82ba9 100644 --- a/tests/flexbuffers_test.h +++ b/tests/flexbuffers_test.h @@ -6,6 +6,7 @@ namespace tests { void FlexBuffersTest(); void FlexBuffersReuseBugTest(); +void FlexBuffersCyclicBufferTest(); void FlexBuffersFloatingPointTest(); void FlexBuffersDeprecatedTest(); void ParseFlexbuffersFromJsonWithNullTest(); diff --git a/tests/fuzzer/flexbuffers_verifier_fuzzer.cc b/tests/fuzzer/flexbuffers_verifier_fuzzer.cc index 80189245a..36680e71b 100644 --- a/tests/fuzzer/flexbuffers_verifier_fuzzer.cc +++ b/tests/fuzzer/flexbuffers_verifier_fuzzer.cc @@ -11,7 +11,14 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { std::vector reuse_tracker; // Check both with and without reuse tracker paths. - flexbuffers::VerifyBuffer(data, size, &reuse_tracker); + if (flexbuffers::VerifyBuffer(data, size, &reuse_tracker)) { + // A buffer that passes verification must be safe to fully traverse. This + // exercises the "verified => safe to access" contract that the verify-only + // path above does not (previously untested), and would catch e.g. cyclic + // buffers that recurse without bound. + std::string s; + flexbuffers::GetRoot(data, size).ToString(true, true, s); + } // FIXME: we can't really verify this path, because the fuzzer will // construct buffers that time out. // Add a simple #define to bound the number of steps just for the fuzzer? diff --git a/tests/test.cpp b/tests/test.cpp index 5a43546f5..02e7b46e4 100644 --- a/tests/test.cpp +++ b/tests/test.cpp @@ -1829,6 +1829,7 @@ int FlatBufferTests(const std::string& tests_data_path) { CreateSharedStringTest(); FlexBuffersTest(); FlexBuffersReuseBugTest(); + FlexBuffersCyclicBufferTest(); FlexBuffersDeprecatedTest(); UninitializedVectorTest(); EqualOperatorTest();