Validate seek table bounds in WaveBankReader

[Roland Shum (ShumWengSang)]

Validate seek table bounds in WaveBankReader

Summary

FindSeekTable returns a pointer into the wave bank's seek-table segment. It validated the offset it was handed but never the subtable that offset points at, and its own bounds checks were incomplete in three ways. The net effect is that values taken straight from the file can steer a read outside the seek segment allocation.

The segment is laid out as dwEntryCount uint32_t offsets, followed by per-entry subtables. Each subtable begins with its own element count. m_seekData is allocated at exactly seekLen bytes (WaveBankReader.cpp:745), and seekSize in FindSeekTable is that same header field, so "inside the segment" and "inside the allocation" are the same condition.

Defects

1. The per-entry offset check tested only the start of the element.

   if ((index * sizeof(uint32_t)) > seekSize)

   At index * 4 == seekSize - 1 this passes, and table[index] then reads past the end. The whole element has to be inside the segment, not just its first byte.

2. The running offset truncated.

   uint32_t offset = table[index];
   offset += sizeof(uint32_t) * data.dwEntryCount;

   sizeof(uint32_t) * data.dwEntryCount is size_t, so on 64-bit the right-hand side is computed in 64 bits and then narrowed into a uint32_t. Both terms come from the file. A sum above UINT32_MAX wraps to a small value that then passes the segment bound check below.

3. The bound check permitted offset == seekSize.

   if (offset > seekSize)

   At equality the returned pointer is exactly one past the end of the allocation, so the leading count word — which every consumer reads first — is itself outside the buffer.

4. The subtable's count was never validated. The count word is attacker-controlled and consumers index [0, count] against it. ENTRYCOMPACT::GetDuration does so directly (seekTable[seekCount], L379 for xWMA and L390 for XMA), and the public GetSeekTable API exports the value outright:

   *pnData  = *seekTable;      // count, unvalidated
   *pData   = seekTable + 1;

   so a caller iterating the returned range reads out of bounds without GetDuration being involved at all.

Three call sites consume FindSeekTable: the XMA2 format path (L957/L987), GetMetadata (L1155/L1158), and GetSeekTable (L1127). Patching GetDuration alone would leave the exported count in GetSeekTable unfixed, so validation belongs in FindSeekTable where all three inherit it. All three already handle a nullptr return.

Change

FindSeekTable now requires the whole offset element to be in the segment, accumulates in 64-bit, requires the leading count word to be fully in the segment before reading it, and validates the declared count against the bytes actually remaining. No signatures change and no new failure mode is introduced — callers already treat nullptr as "no seek table."

Separately, in Open(), the big-endian conversion loop:

for (size_t j = 0; j < seekLen; j += 4, ++ptr)
    *ptr = _byteswap_ulong(*ptr);

steps four bytes at a time to seekLen over a buffer allocated at exactly seekLen bytes, with nothing requiring seekLen to be a multiple of four. When it is not, the final iteration reads and writes up to three bytes past the allocation. The loop now steps over whole elements only. For a well-formed segment the behaviour is identical; the byte-swap path is selected by the file, since Open() accepts either signature and derives be from it (L562/L567).

Validation

Built x64 Release via CMake — no errors, no new warnings.

Both the original and patched FindSeekTable were compiled verbatim into a differential harness (real C++ integer semantics, so the uint32_t truncation behaves exactly as shipped) and swept over 47,784 combinations of segment size, entry count, index, offset value, and subtable count — including the boundary cases at seekSize, offset == seekSize, and offsets chosen to wrap the 32-bit accumulation.

The oracle is the property consumers depend on: for a returned pointer, the count read plus the full [0, count] window must lie inside [0, seekSize).

	count
vectors executed	47,784
original results violating the oracle	3,415
patched results violating the oracle	0
both returned a pointer, identical	2,795
both returned a pointer, different	0
newly rejected by the patch	7,131
newly accepted by the patch	0

The patch removes every unsafe result, never returns a different pointer where both versions accept, and never accepts anything the original rejected — the change is purely additive rejection.

Note for reviewers

The harness also counts misaligned returns: 1,011 for the original, 154 for the patched version. offset originates in the file and nothing requires it to be four-byte aligned, so *seekTable on a const uint32_t* is undefined behaviour — pre-existing in both versions and not introduced here. Adding if (offset & 3) return nullptr; would eliminate it, but those 154 cases are in-bounds and safe, so rejecting them would restrict content beyond what the fix requires. Left out deliberately; happy to add it as a separate change if you'd prefer the stricter contract.