rofs: bound filename reads to the destination buffer - #124
Open
afonsojanu wants to merge 1 commit into
Open
afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
rofs_read_filename() reads a NUL-terminated string byte by byte from the archive with no length check against the caller's buffer. The rofs.dat format has no length prefix for these names, so a corrupt or malicious archive that never supplies a NUL within dst_len bytes makes the loop keep writing past the end of the destination, which is either the 16-byte stack buffer in rofs_load_entry() or the 48-byte name field inside the heap-allocated ROFSinfo struct. Confirmed with a crafted archive under ASan: mounting it produced a heap-buffer-overflow write in rofs_read_filename(), reached straight from PHYSFS_mount() -> ROFS_openArchive() -> rofs_load_header(). The fix bails out with PHYSFS_ERR_CORRUPT once the name would no longer fit, instead of writing outside dst. Added test/test_rofs_corrupt_filename.c, which builds such an archive and checks that PHYSFS_mount() now rejects it cleanly.
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
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.
rofs_read_filename()reads a NUL-terminated string byte by byte fromthe archive, but it never checks the byte count against the
dst_lenargument the caller passed in. The rofs.dat format doesn't have a
length prefix for these names, it's just bytes until a NUL, so a
corrupt or hostile archive that omits the terminator for long enough
makes the loop keep writing past the end of the destination buffer.
That destination is either the 16-byte
shortnamestack buffer inrofs_load_entry(), or the 48-bytenamefield ofdirs[0]/dirs[1]inside the heap-allocated
ROFSinfostruct (read inrofs_load_header(), which runs duringROFS_openArchive()).I confirmed this with a small crafted archive under ASan: just calling
PHYSFS_mount()on it (magic header followed by a long run ofnon-NUL bytes) produces a heap-buffer-overflow write in
rofs_read_filename, called fromrofs_load_header->rofs_load_entries->ROFS_openArchive.The fix bails out with
PHYSFS_ERR_CORRUPTonce the name has used updst_lenbytes without a terminator, instead of writing pastdst.This is the same kind of guard
rofs_load_entry()already has for thecombined
entry.namefield (added in #106), just applied to theper-directory name buffers as well.
Added
test/test_rofs_corrupt_filename.c, wired up next to theexisting
test_physfstarget and registered withctest. It builds aminimal malformed rofs.dat and checks that
PHYSFS_mount()nowrejects it instead of overrunning the buffer; I verified it aborts
under ASan against the old code and passes against the fix.
Test plan
-fsanitize=address,undefined, reproduced theoverflow against the old code.
ctest -R rofs_corrupt_filenamepasses with the fix and reliablycrashes (ASan abort) without it.
cmake --build .still succeeds with all archivers enabled.