From 67e079664c340ea9b65f2c0ccc252d34ae20122f Mon Sep 17 00:00:00 2001 From: Roland Shum Date: Fri, 14 Aug 2026 14:00:00 -0700 Subject: [PATCH] Validate seek table bounds in WaveBankReader FindSeekTable returned a pointer into the seek segment without validating the subtable it points at, and its own bounds checks were incomplete: - The per-entry offset check tested only the start of the element, so an index whose last byte fell outside the segment was accepted. - The running offset was accumulated into a uint32_t from a size_t expression, so it truncated and a wrapped value passed the segment bound check. - The bound check permitted offset == seekSize, yielding a pointer one past the end whose leading count word was itself outside the allocation. - The count word at the head of each subtable was never validated, so callers indexing [0, count] could read far past the segment. GetDuration indexes it directly, and GetSeekTable exports it to callers. Validate centrally in FindSeekTable so all three call sites inherit the result, and accumulate in 64-bit. Callers already handle a nullptr return. Also step the big-endian conversion loop over whole uint32_t elements only: the seek segment length comes from the file and is not guaranteed to be a multiple of four, while the buffer is allocated at exactly that length, so a trailing partial element was read and written out of bounds. --- Audio/WaveBankReader.cpp | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/Audio/WaveBankReader.cpp b/Audio/WaveBankReader.cpp index 5135c99a..f184d6fd 100644 --- a/Audio/WaveBankReader.cpp +++ b/Audio/WaveBankReader.cpp @@ -408,20 +408,33 @@ namespace const uint32_t seekSize = header.Segments[HEADER::SEGIDX_SEEKTABLES].dwLength; - if ((index * sizeof(uint32_t)) > seekSize) + // The segment opens with dwEntryCount uint32_t offsets. Require the whole element + // to be inside the segment, not merely its first byte. + if ((uint64_t(index) + 1u) * sizeof(uint32_t) > seekSize) return nullptr; auto table = reinterpret_cast(seekTable); - uint32_t offset = table[index]; - if (offset == uint32_t(-1)) + const uint32_t entry = table[index]; + if (entry == uint32_t(-1)) return nullptr; - offset += sizeof(uint32_t) * data.dwEntryCount; + // Both terms come from the file and their sum exceeds a uint32_t for large values, + // so accumulate in 64-bit; a truncated sum would wrap past the bounds check below. + const uint64_t offset = uint64_t(entry) + uint64_t(sizeof(uint32_t)) * uint64_t(data.dwEntryCount); - if (offset > seekSize) + // The subtable leads with its own entry count, so that value must itself be fully + // inside the segment before anything may read it. + if ((offset + sizeof(uint32_t)) > seekSize) return nullptr; - return reinterpret_cast(seekTable + offset); + auto result = reinterpret_cast(seekTable + offset); + + // Validate the declared count against the bytes actually remaining. Consumers index + // result[0] through result[*result], so that many elements plus the count must fit. + if ((uint64_t(*result) + 1u) * sizeof(uint32_t) > (uint64_t(seekSize) - offset)) + return nullptr; + + return result; } } @@ -765,8 +778,11 @@ HRESULT WaveBankReader::Impl::Open(const wchar_t* szFileName) noexcept(false) if (be) { + // Step only over whole uint32_t elements: seekLen comes from the file and is + // not guaranteed to be a multiple of 4, while the buffer is exactly seekLen + // bytes, so a trailing partial element would be read and written out of bounds. auto ptr = reinterpret_cast(m_seekData.get()); - for (size_t j = 0; j < seekLen; j += 4, ++ptr) + for (size_t j = 0; (j + sizeof(uint32_t)) <= seekLen; j += 4, ++ptr) { *ptr = _byteswap_ulong(*ptr); }