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.

Author: Asroy-C-Sitorus

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<size_t>((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;
   }

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<uint8_t> 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,

tests/flexbuffers_test.h

@@ -6,6 +6,7 @@ namespace tests {
 
 void FlexBuffersTest();
 void FlexBuffersReuseBugTest();
+void FlexBuffersCyclicBufferTest();
 void FlexBuffersFloatingPointTest();
 void FlexBuffersDeprecatedTest();
 void ParseFlexbuffersFromJsonWithNullTest();

tests/fuzzer/flexbuffers_verifier_fuzzer.cc

@@ -11,7 +11,14 @@
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
   std::vector<uint8_t> 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?

tests/test.cpp

@@ -1829,6 +1829,7 @@ int FlatBufferTests(const std::string& tests_data_path) {
   CreateSharedStringTest();
   FlexBuffersTest();
   FlexBuffersReuseBugTest();
+  FlexBuffersCyclicBufferTest();
   FlexBuffersDeprecatedTest();
   UninitializedVectorTest();
   EqualOperatorTest();