rofs: bound filename reads to the destination buffer

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)

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');
 

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 <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#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 */