Skip to content

rofs: bound filename reads to the destination buffer - #124

Open
afonsojanu wants to merge 1 commit into
icculus:mainfrom
afonsojanu:fix/rofs-read-filename-overflow
Open

afonsojanu wants to merge 1 commit into
icculus:mainfrom
afonsojanu:fix/rofs-read-filename-overflow

Conversation

@afonsojanu

Copy link
Copy Markdown

rofs_read_filename() reads a NUL-terminated string byte by byte from
the archive, but it never checks the byte count against the dst_len
argument 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 shortname stack buffer in
rofs_load_entry(), or the 48-byte name field of dirs[0]/dirs[1]
inside the heap-allocated ROFSinfo struct (read in
rofs_load_header(), which runs during ROFS_openArchive()).

I confirmed this with a small crafted archive under ASan: just calling
PHYSFS_mount() on it (magic header followed by a long run of
non-NUL bytes) produces a heap-buffer-overflow write in
rofs_read_filename, called from rofs_load_header ->
rofs_load_entries -> ROFS_openArchive.

==...==ERROR: AddressSanitizer: heap-buffer-overflow ... WRITE of size 1
    #0 rofs_read_filename physfs_archiver_rofs.c:547
    #1 rofs_load_header physfs_archiver_rofs.c:643
    #2 rofs_load_entries physfs_archiver_rofs.c:669
    #3 ROFS_openArchive physfs_archiver_rofs.c:704
    ...
    #8 PHYSFS_mount physfs.c:1917

The fix bails out with PHYSFS_ERR_CORRUPT once the name has used up
dst_len bytes without a terminator, instead of writing past dst.
This is the same kind of guard rofs_load_entry() already has for the
combined entry.name field (added in #106), just applied to the
per-directory name buffers as well.

Added test/test_rofs_corrupt_filename.c, wired up next to the
existing test_physfs target and registered with ctest. It builds a
minimal malformed rofs.dat and checks that PHYSFS_mount() now
rejects it instead of overrunning the buffer; I verified it aborts
under ASan against the old code and passes against the fix.

Test plan

  • Built with CMake, -fsanitize=address,undefined, reproduced the
    overflow against the old code.
  • ctest -R rofs_corrupt_filename passes with the fix and reliably
    crashes (ASan abort) without it.
  • Full cmake --build . still succeeds with all archivers enabled.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant