Repository navigation
Conversation
c8bfa9d to
18c6f70
Compare
|
any updates ? |
|
Pushed the regression tests that were missing from this PR ( @dbaileychess could you please approve the pending workflow runs for this commit — CI has never executed on this PR (the build jobs are stuck at "Waiting for status to be reported"), likely held for first-contributor approval. Happy to address any feedback. |
Table.Vector and Table.VectorLen follow a chain of offsets - the field
offset, the vector's relative offset and the length it claims - without
checking any step of it against the buffer bounds:
off += t.Pos
off += GetUOffsetT(t.Bytes[off:]) // out-of-bounds read
return int(GetUOffsetT(t.Bytes[off:])) // out-of-bounds read
A buffer that was not run through the verifier can make either of them
read past the end of the slice and panic, which takes down the process
decoding it. An attacker needs a handful of bytes to do so: a length field
of 0x7fffffff is returned verbatim, so generated code that bounds a loop
with it and then indexes the vector crashes.
Resolve the chain through a helper that validates every step, and reject a
length that does not fit in the buffer even for a one-byte element type.
The checks compare against the remaining buffer space instead of adding up
the result: UOffsetT is a uint32, so an attacker can pick a relative offset
that makes `off + rel` (or `off + Pos`) wrap around back into the buffer
and slip past a bounds check performed after the addition.
On failure VectorLen returns 0 and Vector returns 0. 0 can never be the
start of a vector's data - the smallest one starts at byte 4 - and it keeps
the element reads that generated code performs on the result inside the
buffer. Returning len(Bytes), as the Java and Python ports do, is not
enough in Go: t.Bytes[len(Bytes):] is an empty slice, and the
t.Bytes[start+offset:] that follows still panics.
ByteVector walks the same chain of offsets and now shares the helper, which
also closes a wraparound in it that the checks performed after the addition
used to let through.
Checks the cases the runtime fix handles, through the raw accessors and through generated code: a corrupted length, a relative offset past the end of the buffer, a relative offset that wraps the 32 bit sum back into the buffer, a truncated buffer, wild offsets, an empty buffer and an overflowing table position. Keeps the cases from the first version of this check. On the unpatched runtime they panic inside VectorLen/Vector rather than failing an assertion.
|
Update: the patch on this PR is now complete, not just the first @owvr27 asked for updates. Since the original submission, the head moved from
Relation to #9411#9411 also touches and generated code still crashes on the corrupted-length buffer: So this is not a duplicate of #9411's Go part; it is strictly stronger. If the maintainers prefer one Go change, this one supersedes it, and the Python/Java parts of #9411 are unaffected either way. |
|
Correction to my previous comment: #9411 was revised yesterday and its Go Two differences remain, both measured against 1.
Checks that still fail against 2.
Sharing one resolver across Both revisions are fine as fixes for |
Validates every offset that
Table.VectorLenandTable.Vectorread out of the buffer, instead of trusting it.Problem
Both accessors follow a chain of offsets - the field offset, the vector's relative offset, and the length the vector claims - and dereferenced every step without checking it against the buffer bounds:
A FlatBuffer is not necessarily verified before it is read (the Go runtime has no
Verifier), so all three values are attacker-controlled. A handful of bytes is enough to crash the process that decodes it:The 36-byte buffer used by the tests (
monsterWithInventory) reproduces all of it.Fix
VectorLen,VectorandByteVectornow resolve that chain through one helper that validates every step, and reject a length that could not fit in the buffer even for a one-byte element type.The checks compare against the remaining buffer space instead of adding up the result.
UOffsetTis auint32, so an attacker can pick a relative offset that makesoff + rel(oroff + Pos) wrap around back into the buffer and slip past a bounds check performed after the addition - with24 + 0xffffffecthe sum is4, which is well inside the buffer:On failure
VectorLenreturns0andVectorreturns0.0can never be the start of a vector's data - the smallest possible one starts at byte 4 - and it keeps the element reads that generated code performs on the result inside the buffer. Returninglen(Bytes), as the Java and Python ports do, is not enough here:t.Bytes[len(Bytes):]is empty, and thet.Bytes[start+offset:]that follows still panics.ByteVectorresolves the same chain and now shares the helper, which also closes a wraparound in it that the checks performed after the addition let through.Behaviour on valid buffers is unchanged; no allocation is added.
Tests
CheckVectorLenBoundsChecking(extended, called fromTestAll) covers the corrupted length, the relative offset past the end of the buffer, the wrapping relative offset, a truncated buffer, wild offsets, an empty buffer, an overflowing table position, and the valid cases, through the raw accessors and through generated code. The cases from the first version of the check are kept.Against the unpatched runtime the new checks panic inside
VectorLen/Vectorrather than failing an assertion:Table.Offset,Table.UnionandTable.Indirectare unchanged and remain unchecked; a follow-up for those would be welcome. This is hardening for buffers that were not verified - it does not replace theVerifieron untrusted input.Note: CI's
Test Swift Linux (6.2)is currently red onmasterfor an unrelated reason (swift-crypto 5.x resolved by the Swift test job, which does not compile with the 6.2.x toolchains). That is fixed separately in #9412.