From a26f04c11dd86e949e649a8c0a01eeaeae268c26 Mon Sep 17 00:00:00 2001 From: Ladislav Zezula Date: Sun, 21 Apr 2024 20:15:04 +0200 Subject: Added buffer overflow checks to the Huffmann decompression (https://github.com/ladislav-zezula/StormLib/issues/336) --- src/huffman/huff.cpp | 50 +++++++++++++++++++++++++++++++++++--------------- src/huffman/huff.h | 2 +- 2 files changed, 36 insertions(+), 16 deletions(-) (limited to 'src/huffman') diff --git a/src/huffman/huff.cpp b/src/huffman/huff.cpp index 1b81017..bbb8118 100644 --- a/src/huffman/huff.cpp +++ b/src/huffman/huff.cpp @@ -21,6 +21,11 @@ #include "huff.h" +//----------------------------------------------------------------------------- +// Local defined + +#define HUFF_DECOMPRESS_ERROR 0x1FF + //----------------------------------------------------------------------------- // Table of byte-to-weight values @@ -270,46 +275,50 @@ TInputStream::TInputStream(void * pvInBuffer, size_t cbInBuffer) } // Gets one bit from input stream -unsigned int TInputStream::Get1Bit() +bool TInputStream::Get1Bit(unsigned int & BitValue) { - unsigned int OneBit = 0; - // Ensure that the input stream is reloaded, if there are no bits left if(BitCount == 0) { + // Buffer overflow check + if(pbInBuffer >= pbInBufferEnd) + return false; + // Refill the bit buffer BitBuffer = *pbInBuffer++; BitCount = 8; } // Copy the bit from bit buffer to the variable - OneBit = (BitBuffer & 0x01); + BitValue = (BitBuffer & 0x01); BitBuffer >>= 1; BitCount--; - - return OneBit; + return true; } // Gets the whole byte from the input stream. -unsigned int TInputStream::Get8Bits() +bool TInputStream::Get8Bits(unsigned int & ByteValue) { unsigned int dwReloadByte = 0; - unsigned int dwOneByte = 0; // If there is not enough bits to get the value, // we have to add 8 more bits from the input buffer if(BitCount < 8) { + // Buffer overflow check + if(pbInBuffer >= pbInBufferEnd) + return false; + dwReloadByte = *pbInBuffer++; BitBuffer |= dwReloadByte << BitCount; BitCount += 8; } // Return the lowest 8 its - dwOneByte = (BitBuffer & 0xFF); + ByteValue = (BitBuffer & 0xFF); BitBuffer >>= 8; BitCount -= 8; - return dwOneByte; + return true; } // Gets 7 bits from the stream. DOES NOT remove the bits from input stream @@ -344,6 +353,10 @@ void TInputStream::SkipBits(unsigned int dwBitsToSkip) // we have to add 8 more bits from the input buffer if(BitCount < dwBitsToSkip) { + // Buffer overflow check + if(pbInBuffer >= pbInBufferEnd) + return; + dwReloadByte = *pbInBuffer++; BitBuffer |= dwReloadByte << BitCount; BitCount += 8; @@ -726,7 +739,7 @@ unsigned int THuffmannTree::DecodeOneByte(TInputStream * is) { // Just a sanity check if(pFirst == LIST_HEAD()) - return 0x1FF; + return HUFF_DECOMPRESS_ERROR; // We don't have the quick-link item, we need to parse the tree from its root pItem = pFirst; @@ -735,9 +748,14 @@ unsigned int THuffmannTree::DecodeOneByte(TInputStream * is) // Step down the tree until we find a terminal item while(pItem->pChildLo != NULL) { + unsigned int BitValue = 0; + // If the next bit in the compressed stream is set, we get the higher-weight // child. Otherwise, get the lower-weight child. - pItem = is->Get1Bit() ? pItem->pChildLo->pPrev : pItem->pChildLo; + if(!is->Get1Bit(BitValue)) + return HUFF_DECOMPRESS_ERROR; + + pItem = BitValue ? pItem->pChildLo->pPrev : pItem->pChildLo; BitCount++; // If the number of loaded bits reached 7, @@ -852,7 +870,8 @@ unsigned int THuffmannTree::Decompress(void * pvOutBuffer, unsigned int cbOutLen return 0; // Get the compression type from the input stream - CompressionType = is->Get8Bits(); + if(!is->Get8Bits(CompressionType)) + return 0; bIsCmp0 = (CompressionType == 0) ? 1 : 0; // Build the Huffman tree @@ -863,14 +882,15 @@ unsigned int THuffmannTree::Decompress(void * pvOutBuffer, unsigned int cbOutLen while((DecompressedValue = DecodeOneByte(is)) != 0x100) { // Did an error occur? - if(DecompressedValue == 0x1FF) // An error occurred + if(DecompressedValue == HUFF_DECOMPRESS_ERROR) return 0; // Huffman tree needs to be modified if(DecompressedValue == 0x101) { // The decompressed byte is stored in the next 8 bits - DecompressedValue = is->Get8Bits(); + if(!is->Get8Bits(DecompressedValue)) + return 0; if(!InsertNewBranchAndRebalance(pLast->DecompressedValue, DecompressedValue)) return 0; diff --git a/src/huffman/huff.h b/src/huffman/huff.h index cf1ca4c..e6f9e42 100644 --- a/src/huffman/huff.h +++ b/src/huffman/huff.h @@ -28,7 +28,7 @@ class TInputStream public: TInputStream(void * pvInBuffer, size_t cbInBuffer); - unsigned int Get1Bit(); + bool Get1Bit(unsigned int & BitValue); unsigned int Get8Bits(); bool Peek7Bits(unsigned int & Value); void SkipBits(unsigned int BitCount); -- cgit v1.2.3 From c4e3490d729ba42e92803b7f2ef90ed86b0b0eca Mon Sep 17 00:00:00 2001 From: Ladislav Zezula Date: Sun, 21 Apr 2024 20:21:38 +0200 Subject: Added buffer overflow checks to the Sparse decompression (https://github.com/ladislav-zezula/StormLib/issues/337) --- src/huffman/huff.h | 2 +- src/sparse/sparse.cpp | 5 +++++ test/StormTest.cpp | 4 +++- 3 files changed, 9 insertions(+), 2 deletions(-) (limited to 'src/huffman') diff --git a/src/huffman/huff.h b/src/huffman/huff.h index e6f9e42..b2c6370 100644 --- a/src/huffman/huff.h +++ b/src/huffman/huff.h @@ -29,7 +29,7 @@ class TInputStream TInputStream(void * pvInBuffer, size_t cbInBuffer); bool Get1Bit(unsigned int & BitValue); - unsigned int Get8Bits(); + bool Get8Bits(unsigned int & ByteValue); bool Peek7Bits(unsigned int & Value); void SkipBits(unsigned int BitCount); diff --git a/src/sparse/sparse.cpp b/src/sparse/sparse.cpp index 6d1b621..6cf2df2 100644 --- a/src/sparse/sparse.cpp +++ b/src/sparse/sparse.cpp @@ -261,7 +261,12 @@ int DecompressSparse(void * pvOutBuffer, int * pcbOutBuffer, void * pvInBuffer, // If highest bit, it means that that normal data follow if(OneByte & 0x80) { + // Check the length of one chunk. Check for overflows cbChunkSize = (OneByte & 0x7F) + 1; + if((pbInBuffer + cbChunkSize) > pbInBufferEnd) + return 0; + + // Copy the chunk. Make sure that the buffer won't overflow cbChunkSize = (cbChunkSize < cbOutBuffer) ? cbChunkSize : cbOutBuffer; memcpy(pbOutBuffer, pbInBuffer, cbChunkSize); pbInBuffer += cbChunkSize; diff --git a/test/StormTest.cpp b/test/StormTest.cpp index 60e2167..721ee19 100755 --- a/test/StormTest.cpp +++ b/test/StormTest.cpp @@ -3789,7 +3789,7 @@ static void Test_PlayingSpace() { HANDLE hMpq = NULL; - if(SFileOpenArchive(_T("e:\\poc18"), 0, 0, &hMpq)) + if(SFileOpenArchive(_T("e:\\poc21"), 0, 0, &hMpq)) { SFileCompactArchive(hMpq, _T("e:\\Ladik\\Incoming\\poc18"), true); SFileCloseArchive(hMpq); @@ -4008,6 +4008,8 @@ static const TEST_INFO1 Test_OpenMpqs[] = {_T("pocs/MPQ_2024_07_InvalidBitmapFooter.mpq"), NULL, "--------------------------------", TFLG_WILL_FAIL}, {_T("pocs/MPQ_2024_08_InvalidSectorSize.mpq"), NULL, "--------------------------------", TFLG_WILL_FAIL}, {_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}, // Correct or damaged archives {_T("MPQ_1997_v1_Diablo1_DIABDAT.MPQ"), NULL, "554b538541e42170ed41cb236483489e", 2910, &TwoFilesD1}, // Base MPQ from Diablo 1 -- cgit v1.2.3