From 86b6888f3ece894d02ef625ff16939a14670ed98 Mon Sep 17 00:00:00 2001 From: Ladislav Zezula Date: Sun, 21 Apr 2024 14:14:28 +0200 Subject: Fixed heap overrun in https://github.com/ladislav-zezula/StormLib/issues/327 --- src/SBaseFileTable.cpp | 91 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 59 insertions(+), 32 deletions(-) (limited to 'src/SBaseFileTable.cpp') diff --git a/src/SBaseFileTable.cpp b/src/SBaseFileTable.cpp index a222ac5..074501e 100644 --- a/src/SBaseFileTable.cpp +++ b/src/SBaseFileTable.cpp @@ -63,8 +63,8 @@ struct TMPQBits { static TMPQBits * Create(DWORD NumberOfBits, BYTE FillValue); - void GetBits(unsigned int nBitPosition, unsigned int nBitLength, void * pvBuffer, int nResultSize); - void SetBits(unsigned int nBitPosition, unsigned int nBitLength, void * pvBuffer, int nResultSize); + DWORD GetBits(unsigned int nBitPosition, unsigned int nBitLength, void * pvBuffer, int nResultSize); + DWORD SetBits(unsigned int nBitPosition, unsigned int nBitLength, void * pvBuffer, int nResultSize); static const USHORT SetBitsMask[]; // Bit mask for each number of bits (0-8) @@ -94,7 +94,7 @@ TMPQBits * TMPQBits::Create( return pBitArray; } -void TMPQBits::GetBits( +DWORD TMPQBits::GetBits( unsigned int nBitPosition, unsigned int nBitLength, void * pvBuffer, @@ -110,6 +110,12 @@ void TMPQBits::GetBits( // Keep compilers happy for platforms where nResultByteSize is not used STORMLIB_UNUSED(nResultByteSize); + // Check for bit overflow + if(nBitPosition + nBitLength < nBitPosition) + return ERROR_BUFFER_OVERFLOW; + if(nBitPosition + nBitLength > NumberOfBits) + return ERROR_BUFFER_OVERFLOW; + #ifdef _DEBUG // Check if the target is properly zeroed for(int i = 0; i < nResultByteSize; i++) @@ -157,9 +163,10 @@ void TMPQBits::GetBits( *pbBuffer &= (0x01 << nBitLength) - 1; } + return ERROR_SUCCESS; } -void TMPQBits::SetBits( +DWORD TMPQBits::SetBits( unsigned int nBitPosition, unsigned int nBitLength, void * pvBuffer, @@ -175,6 +182,12 @@ void TMPQBits::SetBits( // Keep compilers happy for platforms where nResultByteSize is not used STORMLIB_UNUSED(nResultByteSize); + // Check for bit overflow + if(nBitPosition + nBitLength < nBitPosition) + return ERROR_BUFFER_OVERFLOW; + if(nBitPosition + nBitLength > NumberOfBits) + return ERROR_BUFFER_OVERFLOW; + #ifndef STORMLIB_LITTLE_ENDIAN // Adjust the buffer pointer for big endian platforms pbBuffer += (nResultByteSize - 1); @@ -223,6 +236,7 @@ void TMPQBits::SetBits( Elements[nBytePosition] = (BYTE)((Elements[nBytePosition] & ~AndMask) | BitBuffer); } } + return ERROR_SUCCESS; } void GetMPQBits(TMPQBits * pBits, unsigned int nBitPosition, unsigned int nBitLength, void * pvBuffer, int nResultByteSize) @@ -2599,7 +2613,7 @@ static DWORD BuildFileTable_HetBet(TMPQArchive * ha) TMPQBits * pBitArray; DWORD dwBitPosition = 0; DWORD i; - DWORD dwErrCode = ERROR_FILE_CORRUPT; + DWORD dwErrCode = ERROR_SUCCESS; // Load the BET table from the MPQ pBetTable = LoadBetTable(ha); @@ -2622,10 +2636,16 @@ static DWORD BuildFileTable_HetBet(TMPQArchive * ha) if(pHetTable->pNameHashes[i] != HET_ENTRY_FREE) { // Load the index to the BET table - pHetTable->pBetIndexes->GetBits(pHetTable->dwIndexSizeTotal * i, - pHetTable->dwIndexSize, - &dwFileIndex, - 4); + dwErrCode = pHetTable->pBetIndexes->GetBits(pHetTable->dwIndexSizeTotal * i, + pHetTable->dwIndexSize, + &dwFileIndex, + 4); + if(dwErrCode != ERROR_SUCCESS) + { + FreeBetTable(pBetTable); + return ERROR_FILE_CORRUPT; + } + // Overflow test if(dwFileIndex < pBetTable->dwEntryCount) { @@ -2633,10 +2653,15 @@ static DWORD BuildFileTable_HetBet(TMPQArchive * ha) ULONGLONG NameHash2 = 0; // Load the BET hash - pBetTable->pNameHashes->GetBits(pBetTable->dwBitTotal_NameHash2 * dwFileIndex, - pBetTable->dwBitCount_NameHash2, - &NameHash2, - 8); + dwErrCode = pBetTable->pNameHashes->GetBits(pBetTable->dwBitTotal_NameHash2 * dwFileIndex, + pBetTable->dwBitCount_NameHash2, + &NameHash2, + 8); + if(dwErrCode != ERROR_SUCCESS) + { + FreeBetTable(pBetTable); + return ERROR_FILE_CORRUPT; + } // Combine both part of the name hash and put it to the file table pFileEntry = ha->pFileTable + dwFileIndex; @@ -2653,31 +2678,35 @@ static DWORD BuildFileTable_HetBet(TMPQArchive * ha) DWORD dwFlagIndex = 0; // Read the file position - pBitArray->GetBits(dwBitPosition + pBetTable->dwBitIndex_FilePos, - pBetTable->dwBitCount_FilePos, - &pFileEntry->ByteOffset, - 8); + if((dwErrCode = pBitArray->GetBits(dwBitPosition + pBetTable->dwBitIndex_FilePos, + pBetTable->dwBitCount_FilePos, + &pFileEntry->ByteOffset, + 8)) != ERROR_SUCCESS) + break; // Read the file size - pBitArray->GetBits(dwBitPosition + pBetTable->dwBitIndex_FileSize, - pBetTable->dwBitCount_FileSize, - &pFileEntry->dwFileSize, - 4); + if((dwErrCode = pBitArray->GetBits(dwBitPosition + pBetTable->dwBitIndex_FileSize, + pBetTable->dwBitCount_FileSize, + &pFileEntry->dwFileSize, + 4)) != ERROR_SUCCESS) + break; // Read the compressed size - pBitArray->GetBits(dwBitPosition + pBetTable->dwBitIndex_CmpSize, - pBetTable->dwBitCount_CmpSize, - &pFileEntry->dwCmpSize, - 4); - + if((dwErrCode = pBitArray->GetBits(dwBitPosition + pBetTable->dwBitIndex_CmpSize, + pBetTable->dwBitCount_CmpSize, + &pFileEntry->dwCmpSize, + 4)) != ERROR_SUCCESS) + break; // Read the flag index if(pBetTable->dwFlagCount != 0) { - pBitArray->GetBits(dwBitPosition + pBetTable->dwBitIndex_FlagIndex, - pBetTable->dwBitCount_FlagIndex, - &dwFlagIndex, - 4); + if((dwErrCode = pBitArray->GetBits(dwBitPosition + pBetTable->dwBitIndex_FlagIndex, + pBetTable->dwBitCount_FlagIndex, + &dwFlagIndex, + 4)) != ERROR_SUCCESS) + break; + pFileEntry->dwFlags = pBetTable->pFileFlags[dwFlagIndex]; } @@ -2692,13 +2721,11 @@ static DWORD BuildFileTable_HetBet(TMPQArchive * ha) // Set the current size of the file table FreeBetTable(pBetTable); - dwErrCode = ERROR_SUCCESS; } else { dwErrCode = ERROR_FILE_CORRUPT; } - return dwErrCode; } -- cgit v1.2.3 From 19a8f83554dd0d0f184f6336814f533fc0a85fa2 Mon Sep 17 00:00:00 2001 From: Ladislav Zezula Date: Sun, 21 Apr 2024 14:33:20 +0200 Subject: Fixed stack overflow in https://github.com/ladislav-zezula/StormLib/issues/328 --- src/SBaseCommon.cpp | 16 ++++++---------- src/SBaseFileTable.cpp | 34 ++++++++++++++++++---------------- 2 files changed, 24 insertions(+), 26 deletions(-) (limited to 'src/SBaseFileTable.cpp') diff --git a/src/SBaseCommon.cpp b/src/SBaseCommon.cpp index 587efe4..0de7864 100644 --- a/src/SBaseCommon.cpp +++ b/src/SBaseCommon.cpp @@ -1027,20 +1027,16 @@ void * LoadMpqTable( if(ByteOffset == SFILE_INVALID_POS) FileStream_GetPos(ha->pStream, &ByteOffset); - // On archives v 1.0, hash table and block table can go beyond EOF. + // The hash table and block table can go beyond EOF. // Storm.dll reads as much as possible, then fills the missing part with zeros. // Abused by Spazzler map protector which sets hash table size to 0x00100000 // Abused by NP_Protect in MPQs v4 as well - if(ha->pHeader->wFormatVersion == MPQ_FORMAT_VERSION_1) + FileStream_GetSize(ha->pStream, &FileSize); + if((ByteOffset + dwBytesToRead) > FileSize) { - // Cut the table size - FileStream_GetSize(ha->pStream, &FileSize); - if((ByteOffset + dwBytesToRead) > FileSize) - { - // Fill the extra data with zeros - dwBytesToRead = (DWORD)(FileSize - ByteOffset); - memset(pbMpqTable + dwBytesToRead, 0, (dwTableSize - dwBytesToRead)); - } + // Fill the extra data with zeros + dwBytesToRead = (DWORD)(FileSize - ByteOffset); + memset(pbMpqTable + dwBytesToRead, 0, (dwTableSize - dwBytesToRead)); } // Give the caller information that the table was cut diff --git a/src/SBaseFileTable.cpp b/src/SBaseFileTable.cpp index 074501e..274129d 100644 --- a/src/SBaseFileTable.cpp +++ b/src/SBaseFileTable.cpp @@ -63,8 +63,8 @@ struct TMPQBits { static TMPQBits * Create(DWORD NumberOfBits, BYTE FillValue); - DWORD GetBits(unsigned int nBitPosition, unsigned int nBitLength, void * pvBuffer, int nResultSize); - DWORD SetBits(unsigned int nBitPosition, unsigned int nBitLength, void * pvBuffer, int nResultSize); + DWORD GetBits(unsigned int nBitPosition, unsigned int nBitLength, void * pvBuffer, unsigned int nResultSize); + DWORD SetBits(unsigned int nBitPosition, unsigned int nBitLength, void * pvBuffer, unsigned int nResultSize); static const USHORT SetBitsMask[]; // Bit mask for each number of bits (0-8) @@ -98,7 +98,7 @@ DWORD TMPQBits::GetBits( unsigned int nBitPosition, unsigned int nBitLength, void * pvBuffer, - int nResultByteSize) + unsigned int nResultByteSize) { unsigned char * pbBuffer = (unsigned char *)pvBuffer; unsigned int nBytePosition0 = (nBitPosition / 8); @@ -107,18 +107,17 @@ DWORD TMPQBits::GetBits( unsigned int nBitOffset = (nBitPosition & 0x07); unsigned char BitBuffer; - // Keep compilers happy for platforms where nResultByteSize is not used - STORMLIB_UNUSED(nResultByteSize); - // Check for bit overflow if(nBitPosition + nBitLength < nBitPosition) return ERROR_BUFFER_OVERFLOW; if(nBitPosition + nBitLength > NumberOfBits) return ERROR_BUFFER_OVERFLOW; + if(nByteLength > nResultByteSize) + return ERROR_BUFFER_OVERFLOW; #ifdef _DEBUG // Check if the target is properly zeroed - for(int i = 0; i < nResultByteSize; i++) + for(unsigned int i = 0; i < nResultByteSize; i++) assert(pbBuffer[i] == 0); #endif @@ -170,7 +169,7 @@ DWORD TMPQBits::SetBits( unsigned int nBitPosition, unsigned int nBitLength, void * pvBuffer, - int nResultByteSize) + unsigned int nResultByteSize) { unsigned char * pbBuffer = (unsigned char *)pvBuffer; unsigned int nBytePosition = (nBitPosition / 8); @@ -187,6 +186,8 @@ DWORD TMPQBits::SetBits( return ERROR_BUFFER_OVERFLOW; if(nBitPosition + nBitLength > NumberOfBits) return ERROR_BUFFER_OVERFLOW; + if(nBitLength / 8 > nResultByteSize) + return ERROR_BUFFER_OVERFLOW; #ifndef STORMLIB_LITTLE_ENDIAN // Adjust the buffer pointer for big endian platforms @@ -1618,15 +1619,16 @@ static DWORD GetFileIndex_Het(TMPQArchive * ha, const char * szFileName) DWORD dwFileIndex = 0; // Get the file index - pHetTable->pBetIndexes->GetBits(pHetTable->dwIndexSizeTotal * Index, - pHetTable->dwIndexSize, - &dwFileIndex, - sizeof(DWORD)); - - // Verify the FileNameHash against the entry in the table of name hashes - if(dwFileIndex <= ha->dwFileTableSize && ha->pFileTable[dwFileIndex].FileNameHash == FileNameHash) + if(pHetTable->pBetIndexes->GetBits(pHetTable->dwIndexSizeTotal * Index, + pHetTable->dwIndexSize, + &dwFileIndex, + sizeof(DWORD)) == ERROR_SUCCESS) { - return dwFileIndex; + // Verify the FileNameHash against the entry in the table of name hashes + if(dwFileIndex <= ha->dwFileTableSize && ha->pFileTable[dwFileIndex].FileNameHash == FileNameHash) + { + return dwFileIndex; + } } } -- cgit v1.2.3 From c0d7708350d0e38ee71802f14dd34a1dd9732b31 Mon Sep 17 00:00:00 2001 From: Ladislav Zezula Date: Sun, 21 Apr 2024 16:41:40 +0200 Subject: Fixed too-big-alloc in https://github.com/ladislav-zezula/StormLib/issues/329 --- src/SBaseFileTable.cpp | 10 ++++++++++ test/StormTest.cpp | 8 ++++++-- 2 files changed, 16 insertions(+), 2 deletions(-) (limited to 'src/SBaseFileTable.cpp') diff --git a/src/SBaseFileTable.cpp b/src/SBaseFileTable.cpp index 274129d..84cfe45 100644 --- a/src/SBaseFileTable.cpp +++ b/src/SBaseFileTable.cpp @@ -635,6 +635,8 @@ DWORD ConvertMpqHeaderToFormat4( // Size of the hi-block table if(pHeader->HiBlockTablePos64) { + if(pHeader->HiBlockTablePos64 > FileSize) + return ERROR_FILE_CORRUPT; pHeader->HiBlockTableSize64 = MaxOffset - pHeader->HiBlockTablePos64; MaxOffset = pHeader->HiBlockTablePos64; } @@ -642,6 +644,8 @@ DWORD ConvertMpqHeaderToFormat4( // Size of the block table if(BlockTablePos64) { + if(BlockTablePos64 > FileSize) + return ERROR_FILE_CORRUPT; pHeader->BlockTableSize64 = MaxOffset - BlockTablePos64; MaxOffset = BlockTablePos64; } @@ -649,6 +653,8 @@ DWORD ConvertMpqHeaderToFormat4( // Size of the hash table if(HashTablePos64) { + if(HashTablePos64 > FileSize) + return ERROR_FILE_CORRUPT; pHeader->HashTableSize64 = MaxOffset - HashTablePos64; MaxOffset = HashTablePos64; } @@ -656,6 +662,8 @@ DWORD ConvertMpqHeaderToFormat4( // Size of the BET table if(pHeader->BetTablePos64) { + if(pHeader->BetTablePos64 > FileSize) + return ERROR_FILE_CORRUPT; pHeader->BetTableSize64 = MaxOffset - pHeader->BetTablePos64; MaxOffset = pHeader->BetTablePos64; } @@ -663,6 +671,8 @@ DWORD ConvertMpqHeaderToFormat4( // Size of the HET table if(pHeader->HetTablePos64) { + if(pHeader->HetTablePos64 > FileSize) + return ERROR_FILE_CORRUPT; pHeader->HetTableSize64 = MaxOffset - pHeader->HetTablePos64; // MaxOffset = pHeader->HetTablePos64; } diff --git a/test/StormTest.cpp b/test/StormTest.cpp index 718c910..00c5e13 100755 --- a/test/StormTest.cpp +++ b/test/StormTest.cpp @@ -4002,6 +4002,11 @@ static const TEST_INFO1 TestList_MasterMirror[] = static const TEST_INFO1 Test_OpenMpqs[] = { + // PoC's by Gabe Sherman from FuturesLab + {_T("pocs/MPQ_2024_01_HeapOverrun.mpq"), NULL, "7008f95dcbc4e5d840830c176dec6969", 14}, + {_T("pocs/MPQ_2024_02_StackOverflow.mpq"), NULL, "7093fcbcc9674b3e152e74e8e8a937bb", 4}, + {_T("pocs/MPQ_2024_03_TooBigAlloc.mpq"), NULL, "--------------------------------", TFLG_WILL_FAIL}, + // Correct or damaged archives {_T("MPQ_1997_v1_Diablo1_DIABDAT.MPQ"), NULL, "554b538541e42170ed41cb236483489e", 2910, &TwoFilesD1}, // Base MPQ from Diablo 1 {_T("MPQ_1997_v1_patch_rt_SC1B.mpq"), NULL, "43fe7d362955be68a708486e399576a7", 10}, // From Starcraft 1 BETA @@ -4182,12 +4187,11 @@ static const LPCSTR Test_CreateMpq_Localized[] = //----------------------------------------------------------------------------- // Main -#define TEST_COMMAND_LINE +//#define TEST_COMMAND_LINE //#define TEST_LOCAL_LISTFILE //#define TEST_STREAM_OPERATIONS //#define TEST_MASTER_MIRROR #define TEST_OPEN_MPQ -#define TEST_OPEN_MPQ #define TEST_REOPEN_MPQ #define TEST_VERIFY_SIGNATURE #define TEST_REPLACE_FILE -- cgit v1.2.3 From 34a29066eaf9050ba88fd02cd8a370ac0fc7c7ea Mon Sep 17 00:00:00 2001 From: Ladislav Zezula Date: Sun, 21 Apr 2024 17:07:29 +0200 Subject: Fixed heap overflow in https://github.com/ladislav-zezula/StormLib/issues/331 --- src/SBaseFileTable.cpp | 123 ++++++++++++++++++++++++++++--------------------- test/StormTest.cpp | 4 +- 2 files changed, 73 insertions(+), 54 deletions(-) (limited to 'src/SBaseFileTable.cpp') diff --git a/src/SBaseFileTable.cpp b/src/SBaseFileTable.cpp index 84cfe45..5589320 100644 --- a/src/SBaseFileTable.cpp +++ b/src/SBaseFileTable.cpp @@ -1669,6 +1669,19 @@ void FreeHetTable(TMPQHetTable * pHetTable) //----------------------------------------------------------------------------- // Support for BET table +static bool VerifyBetHeaderSize(TMPQArchive * ha, TMPQBetHeader * pBetHeader) +{ + LPBYTE pbSrcData = (LPBYTE)(pBetHeader + 1); + LPBYTE pbSrcEnd = (LPBYTE)(pBetHeader) + pBetHeader->dwTableSize; + + // Move past the flags + pbSrcData = pbSrcData + (pBetHeader->dwFlagCount * sizeof(DWORD)) + (pBetHeader->dwEntryCount * pBetHeader->dwTableEntrySize); + if(pbSrcData > pbSrcEnd) + return false; + + return true; +} + static void CreateBetHeader( TMPQArchive * ha, TMPQBetHeader * pBetHeader) @@ -1802,68 +1815,72 @@ static TMPQBetTable * TranslateBetTable( // Note that if it's not, it is not a problem //assert(pBetHeader->dwEntryCount == ha->pHetTable->dwEntryCount); - // Create translated table - pBetTable = CreateBetTable(pBetHeader->dwEntryCount); - if(pBetTable != NULL) + // Verify an obviously-wrong values + if(VerifyBetHeaderSize(ha, pBetHeader)) { - // Copy the variables from the header to the BetTable - pBetTable->dwTableEntrySize = pBetHeader->dwTableEntrySize; - pBetTable->dwBitIndex_FilePos = pBetHeader->dwBitIndex_FilePos; - pBetTable->dwBitIndex_FileSize = pBetHeader->dwBitIndex_FileSize; - pBetTable->dwBitIndex_CmpSize = pBetHeader->dwBitIndex_CmpSize; - pBetTable->dwBitIndex_FlagIndex = pBetHeader->dwBitIndex_FlagIndex; - pBetTable->dwBitIndex_Unknown = pBetHeader->dwBitIndex_Unknown; - pBetTable->dwBitCount_FilePos = pBetHeader->dwBitCount_FilePos; - pBetTable->dwBitCount_FileSize = pBetHeader->dwBitCount_FileSize; - pBetTable->dwBitCount_CmpSize = pBetHeader->dwBitCount_CmpSize; - pBetTable->dwBitCount_FlagIndex = pBetHeader->dwBitCount_FlagIndex; - pBetTable->dwBitCount_Unknown = pBetHeader->dwBitCount_Unknown; - - // Since we don't know what the "unknown" is, we'll assert when it's zero - assert(pBetTable->dwBitCount_Unknown == 0); - - // Allocate array for flags - if(pBetHeader->dwFlagCount != 0) + // Create translated table + pBetTable = CreateBetTable(pBetHeader->dwEntryCount); + if(pBetTable != NULL) { - // Allocate array for file flags and load it - pBetTable->pFileFlags = STORM_ALLOC(DWORD, pBetHeader->dwFlagCount); - if(pBetTable->pFileFlags != NULL) + // Copy the variables from the header to the BetTable + pBetTable->dwTableEntrySize = pBetHeader->dwTableEntrySize; + pBetTable->dwBitIndex_FilePos = pBetHeader->dwBitIndex_FilePos; + pBetTable->dwBitIndex_FileSize = pBetHeader->dwBitIndex_FileSize; + pBetTable->dwBitIndex_CmpSize = pBetHeader->dwBitIndex_CmpSize; + pBetTable->dwBitIndex_FlagIndex = pBetHeader->dwBitIndex_FlagIndex; + pBetTable->dwBitIndex_Unknown = pBetHeader->dwBitIndex_Unknown; + pBetTable->dwBitCount_FilePos = pBetHeader->dwBitCount_FilePos; + pBetTable->dwBitCount_FileSize = pBetHeader->dwBitCount_FileSize; + pBetTable->dwBitCount_CmpSize = pBetHeader->dwBitCount_CmpSize; + pBetTable->dwBitCount_FlagIndex = pBetHeader->dwBitCount_FlagIndex; + pBetTable->dwBitCount_Unknown = pBetHeader->dwBitCount_Unknown; + + // Since we don't know what the "unknown" is, we'll assert when it's zero + assert(pBetTable->dwBitCount_Unknown == 0); + + // Allocate array for flags + if(pBetHeader->dwFlagCount != 0) { - LengthInBytes = pBetHeader->dwFlagCount * sizeof(DWORD); - memcpy(pBetTable->pFileFlags, pbSrcData, LengthInBytes); - BSWAP_ARRAY32_UNSIGNED(pBetTable->pFileFlags, LengthInBytes); - pbSrcData += LengthInBytes; + // Allocate array for file flags and load it + pBetTable->pFileFlags = STORM_ALLOC(DWORD, pBetHeader->dwFlagCount); + if(pBetTable->pFileFlags != NULL) + { + LengthInBytes = pBetHeader->dwFlagCount * sizeof(DWORD); + memcpy(pBetTable->pFileFlags, pbSrcData, LengthInBytes); + BSWAP_ARRAY32_UNSIGNED(pBetTable->pFileFlags, LengthInBytes); + pbSrcData += LengthInBytes; + } + + // Save the number of flags + pBetTable->dwFlagCount = pBetHeader->dwFlagCount; } - // Save the number of flags - pBetTable->dwFlagCount = pBetHeader->dwFlagCount; - } + // Load the bit-based file table + pBetTable->pFileTable = TMPQBits::Create(pBetTable->dwTableEntrySize * pBetHeader->dwEntryCount, 0); + if(pBetTable->pFileTable != NULL) + { + LengthInBytes = (pBetTable->pFileTable->NumberOfBits + 7) / 8; + memcpy(pBetTable->pFileTable->Elements, pbSrcData, LengthInBytes); + pbSrcData += LengthInBytes; + } - // Load the bit-based file table - pBetTable->pFileTable = TMPQBits::Create(pBetTable->dwTableEntrySize * pBetHeader->dwEntryCount, 0); - if(pBetTable->pFileTable != NULL) - { - LengthInBytes = (pBetTable->pFileTable->NumberOfBits + 7) / 8; - memcpy(pBetTable->pFileTable->Elements, pbSrcData, LengthInBytes); - pbSrcData += LengthInBytes; - } + // Fill the sizes of BET hash + pBetTable->dwBitTotal_NameHash2 = pBetHeader->dwBitTotal_NameHash2; + pBetTable->dwBitExtra_NameHash2 = pBetHeader->dwBitExtra_NameHash2; + pBetTable->dwBitCount_NameHash2 = pBetHeader->dwBitCount_NameHash2; - // Fill the sizes of BET hash - pBetTable->dwBitTotal_NameHash2 = pBetHeader->dwBitTotal_NameHash2; - pBetTable->dwBitExtra_NameHash2 = pBetHeader->dwBitExtra_NameHash2; - pBetTable->dwBitCount_NameHash2 = pBetHeader->dwBitCount_NameHash2; + // Create and load the array of BET hashes + pBetTable->pNameHashes = TMPQBits::Create(pBetTable->dwBitTotal_NameHash2 * pBetHeader->dwEntryCount, 0); + if(pBetTable->pNameHashes != NULL) + { + LengthInBytes = (pBetTable->pNameHashes->NumberOfBits + 7) / 8; + memcpy(pBetTable->pNameHashes->Elements, pbSrcData, LengthInBytes); + // pbSrcData += LengthInBytes; + } - // Create and load the array of BET hashes - pBetTable->pNameHashes = TMPQBits::Create(pBetTable->dwBitTotal_NameHash2 * pBetHeader->dwEntryCount, 0); - if(pBetTable->pNameHashes != NULL) - { - LengthInBytes = (pBetTable->pNameHashes->NumberOfBits + 7) / 8; - memcpy(pBetTable->pNameHashes->Elements, pbSrcData, LengthInBytes); -// pbSrcData += LengthInBytes; + // Dump both tables +// DumpHetAndBetTable(ha->pHetTable, pBetTable); } - - // Dump both tables -// DumpHetAndBetTable(ha->pHetTable, pBetTable); } } } diff --git a/test/StormTest.cpp b/test/StormTest.cpp index 00c5e13..e8c9bd5 100755 --- a/test/StormTest.cpp +++ b/test/StormTest.cpp @@ -4006,6 +4006,8 @@ static const TEST_INFO1 Test_OpenMpqs[] = {_T("pocs/MPQ_2024_01_HeapOverrun.mpq"), NULL, "7008f95dcbc4e5d840830c176dec6969", 14}, {_T("pocs/MPQ_2024_02_StackOverflow.mpq"), NULL, "7093fcbcc9674b3e152e74e8e8a937bb", 4}, {_T("pocs/MPQ_2024_03_TooBigAlloc.mpq"), NULL, "--------------------------------", TFLG_WILL_FAIL}, + {_T("pocs/MPQ_2024_04_HeapOverflow.mpq"), NULL, "--------------------------------", TFLG_WILL_FAIL}, + {_T("pocs/MPQ_2024_05_HeapOverflow.mpq"), NULL, "0539ae020719654a0ea6e2627a8195f8", 14}, // Correct or damaged archives {_T("MPQ_1997_v1_Diablo1_DIABDAT.MPQ"), NULL, "554b538541e42170ed41cb236483489e", 2910, &TwoFilesD1}, // Base MPQ from Diablo 1 @@ -4187,7 +4189,7 @@ static const LPCSTR Test_CreateMpq_Localized[] = //----------------------------------------------------------------------------- // Main -//#define TEST_COMMAND_LINE +#define TEST_COMMAND_LINE //#define TEST_LOCAL_LISTFILE //#define TEST_STREAM_OPERATIONS //#define TEST_MASTER_MIRROR -- cgit v1.2.3 From 3643858d00d26165404837a1f0f7640a84873c30 Mon Sep 17 00:00:00 2001 From: Ladislav Zezula Date: Sun, 21 Apr 2024 18:22:59 +0200 Subject: Fixed reading big files from non-corrupt MPQs --- src/SBaseFileTable.cpp | 22 ++++++++++++++-------- test/StormTest.cpp | 11 ++++------- 2 files changed, 18 insertions(+), 15 deletions(-) (limited to 'src/SBaseFileTable.cpp') diff --git a/src/SBaseFileTable.cpp b/src/SBaseFileTable.cpp index 5589320..8f5c7b3 100644 --- a/src/SBaseFileTable.cpp +++ b/src/SBaseFileTable.cpp @@ -617,6 +617,9 @@ DWORD ConvertMpqHeaderToFormat4( pHeader->BetTablePos64 = 0; } + // Fixup malformed MPQ header sizes + pHeader->dwHeaderSize = STORMLIB_MIN(pHeader->dwHeaderSize, MPQ_HEADER_SIZE_V3); + // // We need to calculate the compressed size of each table. We assume the following order: // 1) HET table @@ -685,7 +688,6 @@ DWORD ConvertMpqHeaderToFormat4( // Verify header MD5. Header MD5 is calculated from the MPQ header since the 'MPQ\x1A' // signature until the position of header MD5 at offset 0xC0 - // Apparently, Starcraft II only accepts MPQ headers where the MPQ header hash matches // If MD5 doesn't match, we ignore this offset. We also ignore it if there's no MD5 at all if(!IsValidMD5(pHeader->MD5_MpqHeader)) @@ -696,6 +698,9 @@ DWORD ConvertMpqHeaderToFormat4( // Byteswap after header MD5 is verified BSWAP_TMPQHEADER(pHeader, MPQ_FORMAT_VERSION_4); + // Fixup malformed MPQ header sizes + pHeader->dwHeaderSize = MPQ_HEADER_SIZE_V4; + // HiBlockTable must be 0 for archives under 4GB if((pHeader->ArchiveSize64 >> 0x20) == 0 && pHeader->HiBlockTablePos64 != 0) return ERROR_FAKE_MPQ_HEADER; @@ -784,9 +789,12 @@ static bool IsValidHashEntry1(TMPQArchive * ha, TMPQHash * pHash, TMPQBlock * pB pBlock = pBlockTable + MPQ_BLOCK_INDEX(pHash); // Check whether this is an existing file - // Also we do not allow to be file size greater than 2GB - if((pBlock->dwFlags & MPQ_FILE_EXISTS) && (pBlock->dwFSize & 0x80000000) == 0) + if(pBlock->dwFlags & MPQ_FILE_EXISTS) { + // We don't allow to be file size greater than 2GB in malformed archives + if((ha->dwFlags & MPQ_FLAG_MALFORMED) && (pBlock->dwFSize >= 0x80000000)) + return false; + // The begin of the file must be within the archive ByteOffset = FileOffsetFromMpqOffset(ha, pBlock->dwFilePos); return (ByteOffset < ha->FileSize); @@ -1669,17 +1677,14 @@ void FreeHetTable(TMPQHetTable * pHetTable) //----------------------------------------------------------------------------- // Support for BET table -static bool VerifyBetHeaderSize(TMPQArchive * ha, TMPQBetHeader * pBetHeader) +static bool VerifyBetHeaderSize(TMPQArchive * /* ha */, TMPQBetHeader * pBetHeader) { LPBYTE pbSrcData = (LPBYTE)(pBetHeader + 1); LPBYTE pbSrcEnd = (LPBYTE)(pBetHeader) + pBetHeader->dwTableSize; // Move past the flags pbSrcData = pbSrcData + (pBetHeader->dwFlagCount * sizeof(DWORD)) + (pBetHeader->dwEntryCount * pBetHeader->dwTableEntrySize); - if(pbSrcData > pbSrcEnd) - return false; - - return true; + return (pbSrcData <= pbSrcEnd); } static void CreateBetHeader( @@ -3156,6 +3161,7 @@ DWORD SaveMPQTables(TMPQArchive * ha) CalculateDataBlockHash(pHeader, MPQ_HEADER_SIZE_V4 - MD5_DIGEST_SIZE, pHeader->MD5_MpqHeader); // Write the MPQ header to the file + assert(pHeader->dwHeaderSize <= sizeof(SaveMpqHeader)); memcpy(&SaveMpqHeader, pHeader, pHeader->dwHeaderSize); BSWAP_TMPQHEADER(&SaveMpqHeader, MPQ_FORMAT_VERSION_1); BSWAP_TMPQHEADER(&SaveMpqHeader, MPQ_FORMAT_VERSION_2); diff --git a/test/StormTest.cpp b/test/StormTest.cpp index e8c9bd5..540557a 100755 --- a/test/StormTest.cpp +++ b/test/StormTest.cpp @@ -3787,16 +3787,13 @@ static DWORD TestReplaceFile(LPCTSTR szMpqPlainName, LPCTSTR szFilePlainName, LP static void Test_PlayingSpace() { - HANDLE hFile = NULL; + SFILE_FIND_DATA sf; HANDLE hMpq = NULL; - if(SFileOpenArchive(_T("(4)Duskwood.w3m"), 0, 0, &hMpq)) + if(SFileOpenArchive(_T("e:\\poc11"), 0, 0, &hMpq)) { - if(SFileOpenFileEx(hMpq, "war3map.j", 0, &hFile)) - { - SFileSetFileLocale(hFile, 1033); - SFileCloseFile(hFile); - } + SFileFindFirstFile(hMpq, "*", &sf, NULL); + SFileAddWave(hMpq, _T("e:\\Ladik\\Incoming\\poc11"), "poc11", MPQ_FILE_FIX_KEY, 1); SFileCloseArchive(hMpq); } } -- cgit v1.2.3 From b35dc1e4d9d1a70c77a7554df87900486388fe0b Mon Sep 17 00:00:00 2001 From: Ladislav Zezula Date: Sun, 21 Apr 2024 19:59:39 +0200 Subject: Regression tests for the first half of the bugs found by Gabe Sherman --- src/SBaseFileTable.cpp | 2 +- test/StormTest.cpp | 17 +++++++++-------- 2 files changed, 10 insertions(+), 9 deletions(-) (limited to 'src/SBaseFileTable.cpp') diff --git a/src/SBaseFileTable.cpp b/src/SBaseFileTable.cpp index 8f5c7b3..fc9418a 100644 --- a/src/SBaseFileTable.cpp +++ b/src/SBaseFileTable.cpp @@ -1683,7 +1683,7 @@ static bool VerifyBetHeaderSize(TMPQArchive * /* ha */, TMPQBetHeader * pBetHead LPBYTE pbSrcEnd = (LPBYTE)(pBetHeader) + pBetHeader->dwTableSize; // Move past the flags - pbSrcData = pbSrcData + (pBetHeader->dwFlagCount * sizeof(DWORD)) + (pBetHeader->dwEntryCount * pBetHeader->dwTableEntrySize); + pbSrcData = pbSrcData + (pBetHeader->dwFlagCount * sizeof(DWORD)) + (pBetHeader->dwEntryCount * pBetHeader->dwTableEntrySize) / 8; return (pbSrcData <= pbSrcEnd); } diff --git a/test/StormTest.cpp b/test/StormTest.cpp index 290c419..31e9981 100755 --- a/test/StormTest.cpp +++ b/test/StormTest.cpp @@ -3999,12 +3999,13 @@ static const TEST_INFO1 TestList_MasterMirror[] = static const TEST_INFO1 Test_OpenMpqs[] = { + // PoC's by Gabe Sherman from FuturesLab - //{_T("pocs/MPQ_2024_01_HeapOverrun.mpq"), NULL, "7008f95dcbc4e5d840830c176dec6969", 14}, - //{_T("pocs/MPQ_2024_02_StackOverflow.mpq"), NULL, "7093fcbcc9674b3e152e74e8e8a937bb", 4}, - //{_T("pocs/MPQ_2024_03_TooBigAlloc.mpq"), NULL, "--------------------------------", TFLG_WILL_FAIL}, - //{_T("pocs/MPQ_2024_04_HeapOverflow.mpq"), NULL, "--------------------------------", TFLG_WILL_FAIL}, - //{_T("pocs/MPQ_2024_05_HeapOverflow.mpq"), NULL, "0539ae020719654a0ea6e2627a8195f8", 14}, + {_T("pocs/MPQ_2024_01_HeapOverrun.mpq"), NULL, "7008f95dcbc4e5d840830c176dec6969", 14}, + {_T("pocs/MPQ_2024_02_StackOverflow.mpq"), NULL, "7093fcbcc9674b3e152e74e8e8a937bb", 4}, + {_T("pocs/MPQ_2024_03_TooBigAlloc.mpq"), NULL, "--------------------------------", TFLG_WILL_FAIL}, + {_T("pocs/MPQ_2024_04_HeapOverflow.mpq"), NULL, "--------------------------------", TFLG_WILL_FAIL}, + {_T("pocs/MPQ_2024_05_HeapOverflow.mpq"), NULL, "0539ae020719654a0ea6e2627a8195f8", 14}, {_T("pocs/MPQ_2024_06_HeapOverflowReadFile.mpq"), NULL, "d41d8cd98f00b204e9800998ecf8427e", 1}, {_T("pocs/MPQ_2024_07_InvalidBitmapFooter.mpq"), NULL, "--------------------------------", TFLG_WILL_FAIL}, {_T("pocs/MPQ_2024_08_InvalidSectorSize.mpq"), NULL, "--------------------------------", TFLG_WILL_FAIL}, @@ -4191,9 +4192,9 @@ static const LPCSTR Test_CreateMpq_Localized[] = // Main #define TEST_COMMAND_LINE -//#define TEST_LOCAL_LISTFILE -//#define TEST_STREAM_OPERATIONS -//#define TEST_MASTER_MIRROR +#define TEST_LOCAL_LISTFILE +#define TEST_STREAM_OPERATIONS +#define TEST_MASTER_MIRROR #define TEST_OPEN_MPQ #define TEST_REOPEN_MPQ #define TEST_VERIFY_SIGNATURE -- cgit v1.2.3 From 5232da3f6ac96177db4d2d806c37c82fc664af8b Mon Sep 17 00:00:00 2001 From: Ladislav Zezula Date: Sun, 21 Apr 2024 20:30:56 +0200 Subject: Fixed buffer overflow in https://github.com/ladislav-zezula/StormLib/issues/338 --- src/SBaseFileTable.cpp | 4 ++++ test/StormTest.cpp | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) (limited to 'src/SBaseFileTable.cpp') diff --git a/src/SBaseFileTable.cpp b/src/SBaseFileTable.cpp index fc9418a..1ed8140 100644 --- a/src/SBaseFileTable.cpp +++ b/src/SBaseFileTable.cpp @@ -574,6 +574,10 @@ DWORD ConvertMpqHeaderToFormat4( // Fill the rest of the header with zeros memset((LPBYTE)pHeader + MPQ_HEADER_SIZE_V2, 0, sizeof(TMPQHeader) - MPQ_HEADER_SIZE_V2); + // Check position of the Hi-block table + if(pHeader->HiBlockTablePos64 > FileSize) + return ERROR_FILE_CORRUPT; + // Calculate the expected hash table size pHeader->HashTableSize64 = (pHeader->dwHashTableSize * sizeof(TMPQHash)); HashTablePos64 = MAKE_OFFSET64(pHeader->wHashTablePosHi, pHeader->dwHashTablePos); diff --git a/test/StormTest.cpp b/test/StormTest.cpp index 721ee19..3c8051e 100755 --- a/test/StormTest.cpp +++ b/test/StormTest.cpp @@ -3789,9 +3789,8 @@ static void Test_PlayingSpace() { HANDLE hMpq = NULL; - if(SFileOpenArchive(_T("e:\\poc21"), 0, 0, &hMpq)) + if(SFileOpenArchive(_T("e:\\poc24"), 0, 0, &hMpq)) { - SFileCompactArchive(hMpq, _T("e:\\Ladik\\Incoming\\poc18"), true); SFileCloseArchive(hMpq); } } @@ -4010,6 +4009,7 @@ static const TEST_INFO1 Test_OpenMpqs[] = {_T("pocs/MPQ_2024_09_InvalidSectorSize.mpq"), NULL, "--------------------------------", TFLG_WILL_FAIL}, {_T("pocs/MPQ_2024_10_HuffDecompressError.mpq"), NULL, "--------------------------------", TFLG_WILL_FAIL}, {_T("pocs/MPQ_2024_10_SparseDecompressError.mpq"), NULL, "--------------------------------", TFLG_WILL_FAIL}, + {_T("pocs/MPQ_2024_11_HiBlockTablePosInvalid.mpq"), NULL, "--------------------------------", TFLG_WILL_FAIL}, // Correct or damaged archives {_T("MPQ_1997_v1_Diablo1_DIABDAT.MPQ"), NULL, "554b538541e42170ed41cb236483489e", 2910, &TwoFilesD1}, // Base MPQ from Diablo 1 -- cgit v1.2.3 From b47bb8b1909e35e086cff00b64e1b0b60d68156f Mon Sep 17 00:00:00 2001 From: Ladislav Zezula Date: Sun, 21 Apr 2024 21:24:48 +0200 Subject: Regression tests complete --- src/SBaseFileTable.cpp | 2 +- test/StormTest.cpp | 36 +++++++++++++++++++++++++++++------- 2 files changed, 30 insertions(+), 8 deletions(-) (limited to 'src/SBaseFileTable.cpp') diff --git a/src/SBaseFileTable.cpp b/src/SBaseFileTable.cpp index 1ed8140..0461be2 100644 --- a/src/SBaseFileTable.cpp +++ b/src/SBaseFileTable.cpp @@ -574,7 +574,7 @@ DWORD ConvertMpqHeaderToFormat4( // Fill the rest of the header with zeros memset((LPBYTE)pHeader + MPQ_HEADER_SIZE_V2, 0, sizeof(TMPQHeader) - MPQ_HEADER_SIZE_V2); - // Check position of the Hi-block table + // Check position of the hi-block table if(pHeader->HiBlockTablePos64 > FileSize) return ERROR_FILE_CORRUPT; diff --git a/test/StormTest.cpp b/test/StormTest.cpp index 3c8051e..fc84589 100755 --- a/test/StormTest.cpp +++ b/test/StormTest.cpp @@ -3785,14 +3785,36 @@ static DWORD TestReplaceFile(LPCTSTR szMpqPlainName, LPCTSTR szFilePlainName, LP return dwErrCode; } +typedef uint8_t u8; +typedef uint16_t u16; +typedef uint32_t u32; +typedef ULONGLONG u64; +typedef unsigned int usize; +typedef char i8; +typedef short i16; +typedef int i32; +typedef LONGLONG i64; +typedef int isize; +typedef float f32; +typedef double f64; + static void Test_PlayingSpace() { - HANDLE hMpq = NULL; - - if(SFileOpenArchive(_T("e:\\poc24"), 0, 0, &hMpq)) - { - SFileCloseArchive(hMpq); - } +/* + i8 v0_tmp[] = {3, 31, -86, 93, 105, -118, -9, -9, 65, 31, 111, 123, -36, 45, -4, -113, 70, 111, -118, 65, 31, 122, 127, 6, -97, 36, 69, 117, 67, 106, -37, 64, 41, 64, -18, 120, -48, -61, -121, 43, 66, 31, 37, -6, -33, 40, 93, -98, 38, 106, 62, -71, 44, -110, 122, 37, 37, -50, -65, 98, 80, 12, 89, 74, -89, 21, -42, -99, -24, 112, 89, 83, 87, -30, 65, 103, -46, -118, 64, 96, 89, -1, -77, 4, 63, 102, -69, -120, -40, 66, -64, 16, 12, 29, -18, -121, 98, -41, 47, 68, 28, -28, 116, -79, 113, -77, -35, -54, -25, -5, 19, -2, 15, 62, 55, -59, -21, -28, -96, -94, -118, 19, 125, 102, 54, 20, 70, 0,}; // pvOutBuffer + i8 * v0 = (i8 *)HeapAlloc(GetProcessHeap(), 0, sizeof(v0_tmp)); + memcpy(v0, v0_tmp, sizeof v0_tmp); + i8 * v1 = v0; // pvOutBuffer + i32 v2_tmp[] = {129, 0,}; // pcbOutBuffer + i32 * v2 = (i32 *)HeapAlloc(GetProcessHeap(), 0, sizeof(v2_tmp)); + memcpy(v2, v2_tmp, sizeof v2_tmp); + i32 * v3 = v2; // pcbOutBuffer + i8 v4_tmp[] = {38, 17, 14, -64, 78, 79, -10, 115, 120, 103, 94, 9, 6, 21, 25, 43, 127, -17, -115, 118, 53, 76, 13, -105, 39, -11, -12, -35, -74, -114, -112, 75, 54, -100, -44, 68, 61, 7, 85, 96, 103, 103, 106, -95, -85, -113, -78, -78, 43, -39, 59, -58, -15, 35, -63, 125, -5, -13, 110, -66, -39, -97, -23, 37, -25, -105, -57, -128, 77, 7, 54, 95, 116, -128, -114, 13, -54, -17, 12, 126, -102, -83, 101, -103, -17, 78, 40, -103, -75, -55, -86, 23, 24, 87, -73, -123, 107, 112, -30, -77, -16, 93, -49, 84, -109, 78, 106, 62, -68, -61, 4, -121, 112, 94, 36, -39, 9, -59, 123, -49, -91, -111, -2, 32, 117, 30, 23, 0, 0,}; // pvInBuffer + i8 * v4 = (i8 *)HeapAlloc(GetProcessHeap(), 0, sizeof(v4_tmp)); + memcpy(v4, v4_tmp, sizeof v4_tmp); + + SCompCompress(v1, v3, v4, 128, 8, 122, 57); // $target +*/ } //----------------------------------------------------------------------------- @@ -4036,7 +4058,7 @@ static const TEST_INFO1 Test_OpenMpqs[] = {_T("MPQ_2023_v1_BroodWarMap.scx"), NULL, "dd3afa3c2f5e562ce3ca91c0c605a71f", 3}, // Brood War map from StarCraft: Brood War 1.16 {_T("MPQ_2023_v1_Volcanis.scm"), NULL, "522c89ca96d6736427b01f7c80dd626f", 3}, // Map modified with unusual file compression: ZLIB+Huffman {_T("MPQ_2023_v4_UTF8.s2ma"), NULL, "97b7a686650f3307d135e1d1b017a36a", 67}, // Map contaning files with Chinese names (UTF8-encoded) - {_T("MPQ_2023_v1_GreenTD.w3x"), NULL, "477af4ddf11eead1412d7c87cb81b530", 2004}, // Corrupt sector checksum table in file #A0 + {_T("MPQ_2023_v1_GreenTD.w3x"), NULL, "a8d91fc4e52d7c21ff7feb498c74781a", 2004}, // Corrupt sector checksum table in file #A0 {_T("MPQ_2023_v4_1F644C5A.SC2Replay"), NULL, "b225828ffbf5037553e6a1290187caab", 17}, // Corrupt patch info of the "(attributes)" file {_T(""), NULL, "67faeffd0c0aece205ac8b7282d8ad8e", 4697, &MpqUtf8}, // Chinese name of the MPQ -- cgit v1.2.3