Bound ModelLoader::ReadString/ReadBytes against the mapped file size (OOB read on crafted .flm) - #702
Open
professor-moody wants to merge 1 commit into
Conversation
A crafted .flm whose string-length prefix (or ReadBytes length) exceeds the mapped extent causes a read past the mmap during a normal model load, because ReadString/ReadBytes never check the file-declared length against the size the loader already holds. Reject length < 0 and ptr + length > data + size. Fixes the OOB read reachable via WeightMap::LoadFromFile on an untrusted .flm.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Loading a crafted
.flmtriggers an out-of-bounds read:ModelLoader::ReadStringreads a length directly from the file and buildsstd::string(ptr, ptr + length)with no check against the mapped extent (ModelLoaderis constructed withmapped_file->sizebut never consults it);ReadBytesis likewise unchecked. Reachable fromWeightMap::LoadFromFileon file-controlled key/value entries.This PR adds the missing bounds checks (reject
length < 0andptr + length > data + size) using the size the loader already holds. See the accompanying issue #701 for the ASan-witnessed PoC and details.Reported/fixed by Professor Moody (Halo Forge Labs), found with Crucible.