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.
This commit is contained in:
U-DESKTOP-SPFP6AQ\twistedtechre
2026-05-04 08:12:05 +02:00
parent 6c97c3fff4
commit 7939d92b3e
3 changed files with 89 additions and 23 deletions

View File

@@ -604,7 +604,10 @@ static void FreeFDSMemory(void) {
static int SubLoad(FCEUFILE *fp) { static int SubLoad(FCEUFILE *fp) {
struct md5_context md5; 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; uint32_t x;
uint64_t fsize = FCEU_fgetsize(fp); uint64_t fsize = FCEU_fgetsize(fp);
@@ -615,7 +618,8 @@ static int SubLoad(FCEUFILE *fp) {
if (fsize < 16) if (fsize < 16)
return(0); 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, "FDS\x1a", 4)) {
if (!(memcmp(header + 1, "*NINTENDO-HVC*", 14))) { if (!(memcmp(header + 1, "*NINTENDO-HVC*", 14))) {
@@ -645,7 +649,10 @@ static int SubLoad(FCEUFILE *fp) {
md5_starts(&md5); md5_starts(&md5);
for (x = 0; x < TotalSides; x++) { 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_update(&md5, diskdata[x], 65500);
} }
md5_finish(&md5, GameInfo->MD5); md5_finish(&md5, GameInfo->MD5);

View File

@@ -1212,18 +1212,35 @@ int iNESLoad(const char *name, FCEUFILE *fp)
trainerpoo = (uint8_t*)FCEU_gmalloc(512); trainerpoo = (uint8_t*)FCEU_gmalloc(512);
if (!trainerpoo) if (!trainerpoo)
return 0; 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; 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) 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) 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); rom_size_pow2 = uppow2(iNESCart.PRGRomSize);
@@ -1231,7 +1248,8 @@ int iNESLoad(const char *name, FCEUFILE *fp)
return 0; return 0;
memset(ROM, 0xFF, rom_size_pow2); 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) { if (iNESCart.CHRRomSize) {
vrom_size_pow2 = uppow2(iNESCart.CHRRomSize); vrom_size_pow2 = uppow2(iNESCart.CHRRomSize);
@@ -1244,7 +1262,8 @@ int iNESLoad(const char *name, FCEUFILE *fp)
} }
memset(VROM, 0xFF, vrom_size_pow2); 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) { if (iNESCart.miscROMSize) {
@@ -1256,7 +1275,8 @@ int iNESLoad(const char *name, FCEUFILE *fp)
ROM =NULL; ROM =NULL;
return 0; 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); iNESCart.PRGCRC32 = CalcCRC32(0, ROM, iNESCart.PRGRomSize);

View File

@@ -90,7 +90,13 @@ static uint32_t chr_chip_count;
static uint64_t UNIF_PRGROMSize, UNIF_CHRROMSize; 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; uint32_t x = 1;
if (size < minimum) if (size < minimum)
@@ -175,13 +181,21 @@ static int DoMirroring(FCEUFILE *fp) {
FCEU_printf(" Name/Attribute Table Mirroring: %s\n", stuffo[t]); FCEU_printf(" Name/Attribute Table Mirroring: %s\n", stuffo[t]);
} }
} else { } else {
FCEU_printf(" Incorrect Mirroring Chunk Size (%d). Data is:", uchead.info); /* Diagnostic dump for malformed chunks: cap to a sensible
for (i = 0; i < uchead.info; i++) { * 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) if ((t = FCEU_fgetc(fp)) == EOF)
return(0); return(0);
FCEU_printf(" %02x", t); 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; mirrortodo = 0;
} }
return(1); return(1);
@@ -264,11 +278,21 @@ static int CTRL(FCEUFILE *fp) {
if (t & 2) if (t & 2)
GameInfo->input[1] = SI_ZAPPER; GameInfo->input[1] = SI_ZAPPER;
} else { } else {
FCEU_printf(" Incorrect Control Chunk Size (%d). Data is:", uchead.info); /* Same defensive cap as DoMirroring's else-branch: an attacker-
for (i = 0; i < uchead.info; i++) { * controlled chunk size shouldn't be able to spin a 4 GiB
t = FCEU_fgetc(fp); * 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); FCEU_printf(" %02x", t);
} }
if (uchead.info > dump_max
&& FCEU_fseek(fp, uchead.info - dump_max, SEEK_CUR) < 0)
return(0);
FCEU_printf("\n"); FCEU_printf("\n");
GameInfo->input[0] = GameInfo->input[1] = SI_GAMEPAD; 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-", /* The subsequent 4-byte memcmp()s for "NES-", "UNL-", "HVC-", "BTL-",
* "BMC-" require the boardname buffer to be at least 4 bytes long. * "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 * A malformed UNIF file can declare a MAPR chunk with size < 4, which
* would otherwise produce a heap out-of-bounds read. */ * 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); * 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; return 0;
} }
if (!(boardname = (uint8_t*)FCEU_malloc(uchead.info + 1))) if (!(boardname = (uint8_t*)FCEU_malloc(uchead.info + 1)))
return(0); 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; boardname[uchead.info] = 0;
/* strip whitespaces */ /* strip whitespaces */
boardname = (uint8_t*)string_trim_whitespace((char *const)boardname); boardname = (uint8_t*)string_trim_whitespace((char *const)boardname);