From 1acf9ea9df72648414225888129b08d429cd65ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Afonso=20Janu=C3=A1rio?= Date: Mon, 14 Sep 2026 22:22:07 +0100 Subject: [PATCH] rofs: bound filename reads to the destination buffer 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. --- CMakeLists.txt | 6 +++ src/physfs_archiver_rofs.c | 11 +++- test/test_rofs_corrupt_filename.c | 90 +++++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 1 deletion(-) create mode 100644 test/test_rofs_corrupt_filename.c diff --git a/CMakeLists.txt b/CMakeLists.txt index c4d9732..c974a67 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -309,6 +309,12 @@ if(PHYSFS_BUILD_TEST) target_link_libraries(physfshttpd PRIVATE PhysFS::PhysFS) sdl_add_warning_options(physfshttpd WARNING_AS_ERROR ${PHYSFS_WERROR}) endif() + + add_executable(test_rofs_corrupt_filename test/test_rofs_corrupt_filename.c) + target_link_libraries(test_rofs_corrupt_filename PRIVATE PhysFS::PhysFS) + sdl_add_warning_options(test_rofs_corrupt_filename WARNING_AS_ERROR ${PHYSFS_WERROR}) + enable_testing() + add_test(NAME rofs_corrupt_filename COMMAND test_rofs_corrupt_filename) endif() option(PHYSFS_INSTALL "Enable PhysFS installation" ON) diff --git a/src/physfs_archiver_rofs.c b/src/physfs_archiver_rofs.c index a065ab3..44868b0 100644 --- a/src/physfs_archiver_rofs.c +++ b/src/physfs_archiver_rofs.c @@ -534,7 +534,7 @@ static int isRofs(PHYSFS_Io *io) static int rofs_read_filename(PHYSFS_Io *io, char *dst, PHYSFS_uint32 dst_len) { - int i = 0; + PHYSFS_uint32 i = 0; memset(dst, '\0', dst_len); @@ -544,6 +544,15 @@ static int rofs_read_filename(PHYSFS_Io *io, char *dst, PHYSFS_uint32 dst_len) if (!__PHYSFS_readAll(io, &c, 1)) return(0); + /* Filenames in a rofs.dat are just a run of bytes terminated by a + NUL, with no length prefix, so a corrupt (or hostile) archive + could otherwise make us write past the end of dst. */ + if (i >= dst_len) + { + PHYSFS_setErrorCode(PHYSFS_ERR_CORRUPT); + return(0); + } + dst[i] = c; } while (dst[i++] != '\0'); diff --git a/test/test_rofs_corrupt_filename.c b/test/test_rofs_corrupt_filename.c new file mode 100644 index 0000000..a28403e --- /dev/null +++ b/test/test_rofs_corrupt_filename.c @@ -0,0 +1,90 @@ +/** + * Regression test for the ROFS archiver: a directory-name entry in a + * rofs.dat file is just a run of bytes terminated by a NUL byte, with no + * length prefix anywhere in the format. A corrupt (or deliberately hostile) + * archive that omits the terminator for long enough will make + * rofs_read_filename() walk right off the end of its destination buffer, + * which lives inside a heap allocation. This writes such a file and mounts + * it, and expects PHYSFS_mount() to fail cleanly instead of corrupting + * memory. + * + * Please see the file LICENSE.txt in the source's root directory. + */ + +#include +#include +#include + +#include "physfs.h" + +static const char *make_evil_archive(void) +{ + static const char *fname = "test_rofs_corrupt_filename.dat"; + static const unsigned char rofs_id[21] = { + 3, 0, 0, 0, + 1, 0, 0, 0, + 4, 0, 0, 0, + 0, 1, 1, 0, + 0, 4, 0, 0, + 0 + }; + FILE *f = fopen(fname, "wb"); + int i; + + if (!f) + { + fprintf(stderr, "failed to create %s for writing\n", fname); + return NULL; + } + + fwrite(rofs_id, 1, sizeof (rofs_id), f); + + /* This stands in for the first directory name in the header. It's way + longer than the 48 bytes ROFSentry.name has room for, and never + includes a NUL byte, so a naive byte-at-a-time reader will keep + going well past the end of that field. */ + for (i = 0; i < 4096; i++) + fputc('A', f); + fputc('\0', f); + + fclose(f); + return fname; +} /* make_evil_archive */ + +int main(int argc, char **argv) +{ + const char *fname; + int rc; + + if (!PHYSFS_init(argv[0])) + { + fprintf(stderr, "PHYSFS_init failed: %s\n", + PHYSFS_getErrorByCode(PHYSFS_getLastErrorCode())); + return 1; + } /* if */ + + fname = make_evil_archive(); + if (!fname) + { + PHYSFS_deinit(); + return 1; + } /* if */ + + /* This used to overrun a heap buffer while parsing the header; it + should now just reject the archive as corrupt. */ + rc = PHYSFS_mount(fname, NULL, 1); + + remove(fname); + PHYSFS_deinit(); + + if (rc) + { + fprintf(stderr, "PHYSFS_mount unexpectedly succeeded on a " + "corrupt ROFS archive!\n"); + return 1; + } /* if */ + + printf("ok: corrupt ROFS filename was rejected instead of " + "overrunning the buffer.\n"); + return 0; +} /* main */