[Swift] Verify size-prefixed roots from reader position - #9196
[Swift] Verify size-prefixed roots from reader position#9196carrerasdarren-cell wants to merge 1 commit into
Conversation
|
@carrerasdarren-cell Thanks for opening the PR. I was looking at the cpp approach. And it would be better if the swift implementation also follows a similar convention. Where we only skip after we have verified the prefixed value too. It's fine if we duplicate the logic for the verification here getPrefixedSizeCheckedRoot. flatbuffers/include/flatbuffers/verifier.h Line 256 in 81edeb1 flatbuffers/tests/monster_test.cpp Line 609 in 81edeb1 |
Validate the size prefix before advancing ByteBuffer.reader, then use the active reader position consistently for root, file identifier, and returned-object verification. Cover malformed decoy roots, oversized prefixes, short identifiers, and valid prefixed identifiers.
b4a79a7 to
83b8c03
Compare
|
Thanks, updated in |
mustiikhalil
left a comment
mustiikhalil
left a comment
There was a problem hiding this comment.
LGTM! amazing work! one of the comments can be disregarded if not needed
| guard | ||
| position >= 0, | ||
| position <= storage.capacity, | ||
| storage.capacity - position >= size * 2 |
There was a problem hiding this comment.
nit: &* instead of normal *
| throw FlatbuffersErrors.bufferDoesntContainID | ||
| } | ||
| let str = _buffer.readString(at: size, count: size) | ||
| let str = _buffer.readString(at: position + size, count: size) |
| let prefixPosition = byteBuffer.reader | ||
| let prefix: UOffset = try verifier.getValue(at: prefixPosition) | ||
| let availableSize = byteBuffer.size &- UOffset(MemoryLayout<UOffset>.size) | ||
|
|
||
| if requireExactSize { | ||
| guard prefix == availableSize else { | ||
| throw FlatbuffersErrors.prefixedSizeNotEqualToBufferSize | ||
| } | ||
| } else if prefix > availableSize { | ||
| throw FlatbuffersErrors.outOfBounds( | ||
| position: UInt(prefixPosition) | ||
| &+ UInt(MemoryLayout<UOffset>.size) | ||
| &+ UInt(prefix), | ||
| end: byteBuffer.capacity) | ||
| } |
There was a problem hiding this comment.
Not needed; but if you see fit, and the logic is movable. Would it make sense for this to live in the verifier instead?
2 participants
Summary
ByteBuffer.readerpositionProblem
Both size-prefixed checked-root APIs call
skipPrefix()before delegating togetCheckedRoot. The returned table is constructed from the post-prefix reader position, but verification previously followed a root offset from byte zero and checked a file identifier at byte four.A malformed size-prefixed buffer could therefore place a valid decoy root where the verifier looked while returning a different malformed table from the post-prefix root. The same malformed table is rejected when checked directly, but the prefixed wrappers accepted it. Generated accessors or mutators could then operate outside the buffer bounds despite the checked API succeeding.
Fix
Capture the active reader position once and use it consistently for file-identifier verification, root verification, and table construction. The file-identifier bounds check now accounts for that position without wrapping on short buffers.
Validation
swiftcgit diff --checkpasses