From 7939d92b3e930850538ea059159c2d5145c255ca Mon Sep 17 00:00:00 2001 From: "U-DESKTOP-SPFP6AQ\\twistedtechre" Date: Mon, 4 May 2026 08:12:05 +0200 Subject: [PATCH] core: ROM loader hardening - reject malformed iNES/UNIF/FDS inputs Pass 9 audit of the iNES, UNIF, and FDS loaders against malformed ROM input under AddressSanitizer + UndefinedBehaviorSanitizer. The ROM file is the entry point for untrusted input, so any out-of-bounds read, write, integer overflow, or DoS in the parsers is a real attack surface for anyone who downloads ROMs from third-party sites. Two pre-existing crash inputs caught by ASAN ============================================ test_zero_prg.nes : iNES 2.0 header that decodes to PRGRomSize == 0. Master loads it: uppow2(0) = 0, FCEU_malloc(0) returns an implementation-defined small/null buffer, and the first PRG fetch from the reset vector reads past the heap. => heap-buffer-overflow at cart.c:138 CartBR test_mapr_huge_unif.unf : UNIF MAPR chunk with declared size 0xFFFFFFFF. Master computes FCEU_malloc(uchead.info + 1) which overflows uint32_t to FCEU_malloc(0); the subsequent fread of 4 GiB then heap-corrupts and boardname[uchead.info] = 0 finally segfaults. => SEGV at unif.c:345 SetBoardName Plus a UNIF CTRL DoS: a CTRL chunk declaring 4 GiB drives the diagnostic dump loop through 4 billion FCEU_fgetc() calls (and the loop didn't even break on EOF), printf'ing each byte to the log. Pass 9 fuzz inputs reject every one of those cleanly. Bit-exact identical audio + video output to upstream master across nine valid test ROMs (silent, idle, active gameplay, 5-channel max-volume stress, and the per-APU-channel test ROMs). Changes ======= src/ines.c (iNESLoad): * Reject PRGRomSize <= 0. The legacy iNES path already substitutes ROM_size = 256 when the byte is zero, but the iNES 2.0 exponent encoding can still resolve to zero from a malformed header. * Cast PRGRomSize + CHRRomSize to uint64_t before adding. Both fields are int with a 0x40000000 cap from iNES_read_header_info, so the addition can reach 0x80000000 - signed overflow (UB) that wraps negative and then sign-extends bogusly into the uint64_t romSize used for size sanity prints. * Check the FCEU_fread return value on every read (trainer, PRG, CHR, MiscROM) and warn on truncation. The buffers are pre-filled with 0xFF (open bus) or zero, so a short read leaves a sane tail, but the user got no warning that the file was incomplete. src/unif.c: * FixRomSize: change return type from int to uint32_t. The 0x80000000 cap was being implicitly converted from signed int (where it is INT_MIN) to the uint64_t UNIF_PRGROMSize/CHRROMSize callers, sign- extending to 0xFFFFFFFF80000000 - a 16 EiB malloc request. * SetBoardName: cap uchead.info at 256 (in addition to the existing < 4 reject). Real UNIF board-name strings are < 32 characters. Without the upper bound, uchead.info near 0xFFFFFFFF makes (uchead.info + 1) wrap to zero and FCEU_malloc returns a tiny buffer that the subsequent 4 GiB fread heap-overflows. Also check the fread return. * DoMirroring / CTRL: cap the diagnostic dump-and-skip path at 16 bytes and FCEU_fseek past the remainder. Without the cap, a chunk declaring info ~ 0xFFFFFFFF would spin 4 billion FCEU_fgetc/printf iterations - CPU-bound DoS. The CTRL branch also lacked an EOF break, so a truncated stream would keep returning -1 forever. src/fds.c (SubLoad): * Zero-initialise the stack header[16] so partial reads see deterministic zeros rather than stack garbage. The fsize >= 16 guard reduces the chance of a short read but doesn't eliminate it (mid-stream IO error etc). * Check the FCEU_fread return on the header read and on each 65500-byte side read; warn on truncation. Verification ============ ASAN/UBSAN sweep across 18 fuzz inputs: * Every case that crashed master now LOAD_REJECTED on pass 9. * No new UBSAN/ASAN findings introduced (the pre-existing ppu.c:85 left-shift UB shows up identically on both sides). Bit-exact audio dump regression vs master across 9 valid ROMs at 600 frames each: identical output. No behavioral change for legitimate input. --- src/fds.c | 13 +++++++++--- src/ines.c | 38 ++++++++++++++++++++++++++-------- src/unif.c | 61 ++++++++++++++++++++++++++++++++++++++++++++---------- 3 files changed, 89 insertions(+), 23 deletions(-) diff --git a/src/fds.c b/src/fds.c index 4260a3d..afff0e8 100644 --- a/src/fds.c +++ b/src/fds.c @@ -604,7 +604,10 @@ static void FreeFDSMemory(void) { static int SubLoad(FCEUFILE *fp) { struct md5_context md5; - uint8_t header[16]; + /* Zero-initialise so that if FCEU_fread returns short for any reason + * (truncated stream mid-read, IO error etc.) the downstream memcmps and + * header[4] read see deterministic 0s rather than stack garbage. */ + uint8_t header[16] = {0}; uint32_t x; uint64_t fsize = FCEU_fgetsize(fp); @@ -615,7 +618,8 @@ static int SubLoad(FCEUFILE *fp) { if (fsize < 16) return(0); - FCEU_fread(header, 16, 1, fp); + if (FCEU_fread(header, 1, 16, fp) != 16) + return(0); if (memcmp(header, "FDS\x1a", 4)) { if (!(memcmp(header + 1, "*NINTENDO-HVC*", 14))) { @@ -645,7 +649,10 @@ static int SubLoad(FCEUFILE *fp) { md5_starts(&md5); for (x = 0; x < TotalSides; x++) { - FCEU_fread(diskdata[x], 1, 65500, fp); + /* Zero from FCEU_malloc covers any short read here; flag it so + * the user knows a side is incomplete. */ + if (FCEU_fread(diskdata[x], 1, 65500, fp) != 65500) + FCEU_PrintError(" FDS side %u truncated.\n", (unsigned)x); md5_update(&md5, diskdata[x], 65500); } md5_finish(&md5, GameInfo->MD5); diff --git a/src/ines.c b/src/ines.c index 3fa67e5..fcfd416 100644 --- a/src/ines.c +++ b/src/ines.c @@ -1212,26 +1212,44 @@ int iNESLoad(const char *name, FCEUFILE *fp) trainerpoo = (uint8_t*)FCEU_gmalloc(512); if (!trainerpoo) return 0; - FCEU_fread(trainerpoo, 512, 1, fp); + if (FCEU_fread(trainerpoo, 1, 512, fp) != 512) + FCEU_PrintError(" Trainer block truncated; remaining bytes left zero.\n"); filesize -= 512; } - romSize = iNESCart.PRGRomSize + iNESCart.CHRRomSize; + /* Reject a header that claims no PRG ROM at all. iNES_read_header_info + * applies "ROM_size = 256" on the legacy path when the byte is zero, but + * the iNES 2.0 path can yield PRGRomSize == 0 from a malformed exponent + * encoding. Without PRG bytes, there is nothing to execute and uppow2(0) + * returns 0, which would feed FCEU_malloc(0) (implementation-defined) + * and downstream cart-mapping arithmetic. */ + if (iNESCart.PRGRomSize <= 0) + { + FCEU_PrintError(" Header reports zero PRG ROM size; refusing to load.\n"); + return 0; + } + + /* Cast to uint64_t before adding to avoid signed int overflow when both + * sizes approach the per-side cap of 0x40000000 set by + * iNES_read_header_info. The result feeds the file-length sanity prints + * as well as later signed/unsigned comparisons. */ + romSize = (uint64_t)iNESCart.PRGRomSize + (uint64_t)iNESCart.CHRRomSize; if (romSize > filesize) { - FCEU_PrintError(" File length is too short to contain all data reported from header by %llu\n", romSize - filesize); + FCEU_PrintError(" File length is too short to contain all data reported from header by %llu\n", (unsigned long long)(romSize - filesize)); } else if (romSize < filesize) - FCEU_PrintError(" File contains %llu bytes of unused data\n", filesize - romSize); + FCEU_PrintError(" File contains %llu bytes of unused data\n", (unsigned long long)(filesize - romSize)); rom_size_pow2 = uppow2(iNESCart.PRGRomSize); - + if ((ROM = (uint8_t*)FCEU_malloc(rom_size_pow2)) == NULL) return 0; memset(ROM, 0xFF, rom_size_pow2); - FCEU_fread(ROM, 1, iNESCart.PRGRomSize, fp); + if (FCEU_fread(ROM, 1, iNESCart.PRGRomSize, fp) != (size_t)iNESCart.PRGRomSize) + FCEU_PrintError(" PRG ROM block truncated; remaining bytes left as 0xFF (open bus).\n"); if (iNESCart.CHRRomSize) { vrom_size_pow2 = uppow2(iNESCart.CHRRomSize); @@ -1244,9 +1262,10 @@ int iNESLoad(const char *name, FCEUFILE *fp) } memset(VROM, 0xFF, vrom_size_pow2); - FCEU_fread(VROM, 1, iNESCart.CHRRomSize, fp); + if (FCEU_fread(VROM, 1, iNESCart.CHRRomSize, fp) != (size_t)iNESCart.CHRRomSize) + FCEU_PrintError(" CHR ROM block truncated; remaining bytes left as 0xFF.\n"); } - + if (iNESCart.miscROMSize) { MiscROM =(uint8_t*) FCEU_malloc(iNESCart.miscROMSize); if (!MiscROM) { @@ -1256,7 +1275,8 @@ int iNESLoad(const char *name, FCEUFILE *fp) ROM =NULL; return 0; } - FCEU_fread(MiscROM, 1, iNESCart.miscROMSize, fp); + if (FCEU_fread(MiscROM, 1, iNESCart.miscROMSize, fp) != (size_t)iNESCart.miscROMSize) + FCEU_PrintError(" Misc ROM block truncated; remaining bytes left zero.\n"); } iNESCart.PRGCRC32 = CalcCRC32(0, ROM, iNESCart.PRGRomSize); diff --git a/src/unif.c b/src/unif.c index 76e7146..354f036 100644 --- a/src/unif.c +++ b/src/unif.c @@ -90,7 +90,13 @@ static uint32_t chr_chip_count; static uint64_t UNIF_PRGROMSize, UNIF_CHRROMSize; -static int FixRomSize(uint32_t size, uint32_t minimum) { +/* Returns the next power of two >= max(size, minimum), capped at 0x80000000. + * Returning uint32_t (not int) matters because callers store the result back + * into a uint64_t variable: a signed-int cap of 0x80000000 is INT_MIN, and + * widening to uint64_t sign-extends to 0xFFFFFFFF80000000, which then drives + * a 16-EiB malloc. Defensive even though the cap is unreachable in practice + * given LoadPRG/LoadCHR's own per-chunk caps. */ +static uint32_t FixRomSize(uint32_t size, uint32_t minimum) { uint32_t x = 1; if (size < minimum) @@ -175,13 +181,21 @@ static int DoMirroring(FCEUFILE *fp) { FCEU_printf(" Name/Attribute Table Mirroring: %s\n", stuffo[t]); } } else { - FCEU_printf(" Incorrect Mirroring Chunk Size (%d). Data is:", uchead.info); - for (i = 0; i < uchead.info; i++) { + /* Diagnostic dump for malformed chunks: cap to a sensible + * maximum so a chunk that declares info ~= 0xFFFFFFFF doesn't + * make us spin printf'ing the whole file. Skip past any + * remaining bytes so the chunk loop continues correctly. */ + uint32_t dump_max = uchead.info < 16 ? uchead.info : 16; + FCEU_printf(" Incorrect Mirroring Chunk Size (%u). Data is:", (unsigned)uchead.info); + for (i = 0; i < dump_max; i++) { if ((t = FCEU_fgetc(fp)) == EOF) return(0); FCEU_printf(" %02x", t); } - FCEU_printf("\n Default Name/Attribute Table Mirroring: Horizontal\n", uchead.info); + if (uchead.info > dump_max + && FCEU_fseek(fp, uchead.info - dump_max, SEEK_CUR) < 0) + return(0); + FCEU_printf("\n Default Name/Attribute Table Mirroring: Horizontal\n"); mirrortodo = 0; } return(1); @@ -264,11 +278,21 @@ static int CTRL(FCEUFILE *fp) { if (t & 2) GameInfo->input[1] = SI_ZAPPER; } else { - FCEU_printf(" Incorrect Control Chunk Size (%d). Data is:", uchead.info); - for (i = 0; i < uchead.info; i++) { - t = FCEU_fgetc(fp); + /* Same defensive cap as DoMirroring's else-branch: an attacker- + * controlled chunk size shouldn't be able to spin a 4 GiB + * diagnostic dump, and the missing EOF check let FCEU_fgetc + * return -1 forever on a truncated stream. Cap, log, and seek + * past the rest. */ + uint32_t dump_max = uchead.info < 16 ? uchead.info : 16; + FCEU_printf(" Incorrect Control Chunk Size (%u). Data is:", (unsigned)uchead.info); + for (i = 0; i < dump_max; i++) { + if ((t = FCEU_fgetc(fp)) == EOF) + return(0); FCEU_printf(" %02x", t); } + if (uchead.info > dump_max + && FCEU_fseek(fp, uchead.info - dump_max, SEEK_CUR) < 0) + return(0); FCEU_printf("\n"); GameInfo->input[0] = GameInfo->input[1] = SI_GAMEPAD; } @@ -334,14 +358,29 @@ static int SetBoardName(FCEUFILE *fp) { /* The subsequent 4-byte memcmp()s for "NES-", "UNL-", "HVC-", "BTL-", * "BMC-" require the boardname buffer to be at least 4 bytes long. * A malformed UNIF file can declare a MAPR chunk with size < 4, which - * would otherwise produce a heap out-of-bounds read. */ - if (uchead.info < 4) { - FCEU_PrintError(" MAPR chunk too small (%u bytes); ignoring.\n", (unsigned)uchead.info); + * would otherwise produce a heap out-of-bounds read. + * + * Also cap the upper bound. Real UNIF board names are short (~< 32 + * chars). A malformed chunk that declares uchead.info near 0xFFFFFFFF + * causes (uchead.info + 1) to overflow uint32_t back to 0, so + * FCEU_malloc(0) would return a tiny buffer (or NULL) and the + * subsequent FCEU_fread would attempt a 4 GiB write into it - heap + * corruption that ASAN catches as a SEGV in unif.c when the trailing + * boardname[uchead.info] = 0 store dereferences past the buffer. */ + if (uchead.info < 4 || uchead.info > 256) { + FCEU_PrintError(" MAPR chunk size %u out of range; ignoring.\n", (unsigned)uchead.info); return 0; } if (!(boardname = (uint8_t*)FCEU_malloc(uchead.info + 1))) return(0); - FCEU_fread(boardname, 1, uchead.info, fp); + /* Short reads leave the rest as the zero from FCEU_malloc, so the + * trailing NUL keeps the string well-formed; treat the truncation as + * a parse error rather than letting downstream board lookup match + * a garbage prefix. */ + if (FCEU_fread(boardname, 1, uchead.info, fp) != uchead.info) { + FCEU_PrintError(" MAPR chunk truncated.\n"); + return(0); + } boardname[uchead.info] = 0; /* strip whitespaces */ boardname = (uint8_t*)string_trim_whitespace((char *const)boardname);