Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Vector<T>::Mutate(), Vector<T>::MutateOffset() (vector.h) and Array<T,length>::Mutate()/MutateImpl() (array.h) accepted a caller-supplied runtime index `i` and guarded it only with FLATBUFFERS_ASSERT(i < size()) before writing via WriteScalar (or, for the struct/non-scalar Array specialization, via GetMutablePointer()). FLATBUFFERS_ASSERT compiles to a no-op assert() when NDEBUG is defined, which is the normal configuration for any optimized release build. In that configuration an out-of-range index reaching any of these functions was a genuine, unmitigated heap out-of-bounds write. Fix: keep the existing assert for early detection in debug builds, and add an unconditional bounds check ahead of every write. Each affected function's return type changes from void to bool; an out-of-range index now returns false and performs no write, instead of relying on UB. All existing call sites in this tree discard the return value, which remains valid, so this is source-compatible; as header-only templates, there is no ABI to break either. Functions changed: - Vector<T,SizeT>::Mutate(SizeT, const T&) (vector.h) - Vector<T,SizeT>::MutateOffset(SizeT, const uint8_t*) (vector.h) - Array<T,length>::Mutate(uoffset_t, const T&) (array.h) - Array<T,length>::MutateImpl(true_type, ...) [scalar] (array.h) - Array<T,length>::MutateImpl(false_type, ...) [struct] (array.h) Table::SetField() and generated mutate_<field>() wrappers are unaffected -- those already use compile-time VT_ constants, not a caller-supplied runtime index. Adds MutateBoundsCheckTest (tests/test.cpp) covering in-range and out-of-range Mutate()/MutateOffset() calls; the out-of-range assertions are gated on NDEBUG so the test exercises the fail-closed path the fix adds (the debug-build assert path is unchanged and intentionally still aborts there).
516f0ba to
ca5f428
Compare
|
I believe this is a security advisory. I followed the link under issues, that said security advisories were coordinated with Google VRP. But the google vrp team said I had to submit a PR first. I did not see a way to make this private. Although this is an easy one, I don't like making security advisories public until they are patched. Google VRP was not helpful in responding to requests about that -- seems like it was an AI bot responding. |
Summary
Vector<T>::Mutate(),Vector<T>::MutateOffset()(include/flatbuffers/vector.h) andArray<T,length>::Mutate()/MutateImpl()(include/flatbuffers/array.h) accept a caller-supplied indexiand write to the buffer at that index, guarded only by:FLATBUFFERS_ASSERT(i < size());FLATBUFFERS_ASSERTcompiles to a plainassert()unless overridden, andassert()is compiled out entirely whenNDEBUGis defined -- the standard configuration for an optimized release build (-O2 -DNDEBUG, CMakeRelease/RelWithDebInfo, etc.). In that configuration, these functions become uncheckeddata()[i] = valwrites: any out-of-range index reachingMutate/MutateOffsetis a heap out-of-bounds write, with the offset and (forMutate) the written value both attacker/caller-influenced if the index comes from external input -- which is a realistic and even encouraged usage pattern for this API (mutating a parsed buffer's fields based on external data, per the library's own mutation documentation).This does not affect
Table::SetField()or generatedmutate_<field>()wrappers, which use compile-timeVT_*constants rather than a caller-supplied runtime index, and are out of scope for this change.Fix
Each affected function keeps its existing
FLATBUFFERS_ASSERT(i < size())(useful for catching this early in debug builds) and adds an unconditional runtime bounds check ahead of the write:An out-of-range call now fails closed -- returns
falseand performs no write -- instead of relying on undefined behavior. This applies uniformly whether or notNDEBUGis defined, so the protection holds in release builds where the assert previously provided none.Functions changed:
Vector<T,SizeT>::Mutate(SizeT, const T&)Vector<T,SizeT>::MutateOffset(SizeT, const uint8_t*)Array<T,length>::Mutate(uoffset_t, const T&)Array<T,length>::MutateImpl(true_type, ...)(scalar element path)Array<T,length>::MutateImpl(false_type, ...)(struct element path, viaGetMutablePointer)Compatibility
Each of these functions' return type changes from
voidtobool. This is source-compatible: every existing in-tree call site discards the return value as a bare statement, which is still valid once the function returns something. There is no ABI to break either, since these are header-only templates instantiated and compiled per translation unit by each consumer. No function signature otherwise changes (parameter types/order, overload set, and semantics for an in-range index are all unchanged), and no other behavior changes.Testing
Added
MutateBoundsCheckTest()(tests/test.cpp) covering:Mutate()/MutateOffset()calls still returntrueand write correctly (no regression).Mutate()/MutateOffset()calls (size() + 10) now returnfalse, perform no write, and leave the buffer unchanged, verified under-DNDEBUG(the configuration where this previously had no protection at all) and under AddressSanitizer.Ran the full existing test suite before and after this change, in both a debug+ASan build and a release (
-O2 -DNDEBUG) build, before and after the fix: all tests pass in every configuration, with no regressions. Re-confirmed 2026-09-25 against current master with the same result (one unrelated, pre-existing LeakSanitizer report present identically in both the before and after trees -- see the accompanying SUMMARY for detail; not a functional test failure and not introduced by this change).Notes for reviewers
MutateSafe/GetSafe/GetOptionalto these same two files) does not touchMutate/MutateOffset/MutateImpl-- confirmed via direct diff against master, not assumed. No overlap with this fix.Reported via Google Bug Hunters, issue 564216929.