Skip to content

verifier: bounds check start offset in VerifyBufferFromStart and prevent overflow in VerifySizePrefixedBuffer - #9275

Open
filtede98 wants to merge 1 commit into
google:masterfrom
filtede98:fix-verifier-buffer-from-start-bounds
Open

filtede98 wants to merge 1 commit into
google:masterfrom
filtede98:fix-verifier-buffer-from-start-bounds

Conversation

@filtede98

Copy link
Copy Markdown

Summary

This PR addresses boundary checking and overflow safety in the core flatbuffers::Verifier:

  1. Bounds Check start Offset in VerifyBufferFromStart():
    When verifying buffers that start at an offset (such as size-prefixed buffers where start = sizeof(SizeT) or custom offsets), VerifyBufferFromStart() previously checked:

    if (!Check(size_ >= FLATBUFFERS_MIN_BUFFER_SIZE)) return false;
    if (identifier && !Check((size_ >= 2 * sizeof(flatbuffers::uoffset_t) &&
                              BufferHasIdentifier(buf_ + start, identifier)))) {
      return false;
    }

    Checking size_ >= ... without accounting for start did not guarantee that the remaining buffer starting at buf_ + start has sufficient length. When verifying a buffer with an identifier where start > 0, BufferHasIdentifier(buf_ + start, identifier) reads bytes up to buf_ + start + 2 * sizeof(uoffset_t). If the remaining buffer after start is smaller than 8 bytes, this could read past the end of the buffer.

    We now validate that start <= size_ && size_ - start >= FLATBUFFERS_MIN_BUFFER_SIZE and that start <= size_ && size_ - start >= 2 * sizeof(flatbuffers::uoffset_t) before calling BufferHasIdentifier().

  2. Prevent Integer Overflow in VerifySizePrefixedBuffer():
    In VerifySizePrefixedBuffer(), the check:

    Check(ReadScalar<SizeT>(buf_) + sizeof(SizeT) <= size_)

    could wrap around if ReadScalar<SizeT>(buf_) is close to the integer maximum (e.g. SIZE_MAX). We change this to:

    Check(ReadScalar<SizeT>(buf_) <= size_ - sizeof(SizeT))

    which is immune to integer overflow since Verify<SizeT>(0U) already guarantees sizeof(SizeT) <= size_.

  3. Macro Precedence in base.h:
    Wrapped the FLATBUFFERS_MIN_BUFFER_SIZE macro expression in parentheses to avoid operator precedence hazards when used in composite expressions.

  4. Unit Tests:
    Added assertions in SizePrefixedTest() in tests/monster_test.cpp verifying that truncated size-prefixed buffers and boundary offsets are safely rejected.

…ent overflow in VerifySizePrefixedBuffer

1. In VerifyBufferFromStart(), validate that the remaining buffer starting from start has at least FLATBUFFERS_MIN_BUFFER_SIZE (start <= size_ && size_ - start >= FLATBUFFERS_MIN_BUFFER_SIZE) and at least 2 * sizeof(uoffset_t) when an identifier is present. Previously, only size_ >= ... was checked without accounting for start, which could allow out-of-bounds reads in BufferHasIdentifier() when verifying size-prefixed buffers or custom start offsets with identifiers.

2. In VerifySizePrefixedBuffer(), check ReadScalar<SizeT>(buf_) <= size_ - sizeof(SizeT) to prevent unsigned integer overflow when adding sizeof(SizeT) to a large prefix value.

3. In base.h, wrap the FLATBUFFERS_MIN_BUFFER_SIZE macro expression in parentheses to eliminate operator precedence hazards.

4. In monster_test.cpp, add unit test assertions in SizePrefixedTest() verifying truncated size-prefixed buffers and boundary offsets are safely rejected.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant