From 92028d4e804a7c2f00aead2bbcb8b116c587de25 Mon Sep 17 00:00:00 2001 From: nillerusr Date: Fri, 7 Apr 2023 14:56:56 +0300 Subject: [PATCH] tier1: fix possible unsafe read in lzss SafeUncompress, make tests pass under asan --- engine/common.cpp | 2 +- gameui/BasePanel.cpp | 2 +- public/tier0/platform.h | 10 ++++ public/tier1/lzss.h | 2 +- tier1/lzss.cpp | 44 +++++++++--------- unittests/tier1test/lzsstest.cpp | 78 ++++++++++++++++++++++++++++++++ unittests/tier1test/wscript | 2 +- 7 files changed, 115 insertions(+), 25 deletions(-) create mode 100644 unittests/tier1test/lzsstest.cpp diff --git a/engine/common.cpp b/engine/common.cpp index c0646734..96a5c7f5 100644 --- a/engine/common.cpp +++ b/engine/common.cpp @@ -1385,7 +1385,7 @@ bool COM_BufferToBufferDecompress( void *dest, unsigned int *destLen, const void if ( pHeader->id == LZSS_ID ) { CLZSS s; - int nActualDecompressedSize = s.SafeUncompress( (byte *)source, (byte *)dest, *destLen ); + int nActualDecompressedSize = s.SafeUncompress( (byte *)source, sourceLen, (byte *)dest, *destLen ); if ( nActualDecompressedSize != nDecompressedSize ) { Warning( "NET_BufferToBufferDecompress: header said %d bytes would be decompressed, but we LZSS decompressed %d\n", nDecompressedSize, nActualDecompressedSize ); diff --git a/gameui/BasePanel.cpp b/gameui/BasePanel.cpp index a5336366..b9f0bb07 100644 --- a/gameui/BasePanel.cpp +++ b/gameui/BasePanel.cpp @@ -915,7 +915,7 @@ CBasePanel::CBasePanel() : Panel(NULL, "BaseGameUIPanel") } } -// if( IsAndroid() ) + if( IsAndroid() ) { AddUrlButton( this, "vgui/\x64\x69\x73\x63\x6f\x72\x64\x5f\x6c\x6f\x67\x6f", "\x68\x74\x74\x70\x73\x3a\x2f\x2f\x64\x69\x73\x63\x6f\x72\x64\x2e\x67\x67\x2f\x68\x5a\x52\x42\x37\x57\x4d\x67\x47\x77" ); AddUrlButton( this, "vgui/\x74\x77\x69\x74\x74\x65\x72\x5f\x6c\x6f\x67\x6f", "\x68\x74\x74\x70\x73\x3a\x2f\x2f\x74\x77\x69\x74\x74\x65\x72\x2e\x63\x6f\x6d\x2f\x6e\x69\x6c\x6c\x65\x72\x75\x73\x72" ); diff --git a/public/tier0/platform.h b/public/tier0/platform.h index 4a4c4883..99ac5b3b 100644 --- a/public/tier0/platform.h +++ b/public/tier0/platform.h @@ -606,6 +606,16 @@ typedef void * HINSTANCE; #define FMTFUNCTION( a, b ) #endif +#if defined(__has_feature) +#if __has_feature(address_sanitizer) +#define USING_ASAN 1 +#endif +#endif + +#if !defined( USING_ASAN ) && defined( __SANITIZE_ADDRESS__ ) +#define USING_ASAN 1 +#endif + #if COMPILER_CLANG || COMPILER_GCC #define NO_ASAN __attribute__((no_sanitize("address"))) #else diff --git a/public/tier1/lzss.h b/public/tier1/lzss.h index 8b3409a9..0a1f3002 100644 --- a/public/tier1/lzss.h +++ b/public/tier1/lzss.h @@ -30,7 +30,7 @@ public: unsigned char* CompressNoAlloc( const unsigned char *pInput, int inputlen, unsigned char *pOutput, unsigned int *pOutputSize ); unsigned int Uncompress( const unsigned char *pInput, unsigned char *pOutput ); //unsigned int Uncompress( unsigned char *pInput, CUtlBuffer &buf ); - unsigned int SafeUncompress( const unsigned char *pInput, unsigned char *pOutput, unsigned int unBufSize ); + unsigned int SafeUncompress( const unsigned char *pInput, unsigned int inputSize, unsigned char *pOutput, unsigned int unBufSize ); static bool IsCompressed( const unsigned char *pInput ); static unsigned int GetActualSize( const unsigned char *pInput ); diff --git a/tier1/lzss.cpp b/tier1/lzss.cpp index 23bdead8..4e6138b1 100644 --- a/tier1/lzss.cpp +++ b/tier1/lzss.cpp @@ -294,59 +294,61 @@ unsigned int CLZSS::Uncompress( unsigned char *pInput, CUtlBuffer &buf ) } */ -unsigned int CLZSS::SafeUncompress( const unsigned char *pInput, unsigned char *pOutput, unsigned int unBufSize ) +unsigned int CLZSS::SafeUncompress( const unsigned char *pInput, unsigned int inputSize, unsigned char *pOutput, unsigned int unBufSize ) { unsigned int totalBytes = 0; int cmdByte = 0; int getCmdByte = 0; unsigned int actualSize = GetActualSize( pInput ); - if ( !actualSize ) - { - // unrecognized - return 0; - } - if ( actualSize > unBufSize ) - { + if ( !actualSize || + actualSize > unBufSize || + inputSize <= sizeof( lzss_header_t ) ) return 0; - } + + const unsigned char *pInputEnd = pInput+inputSize-1; + const unsigned char *pOrigOutput = pOutput; pInput += sizeof( lzss_header_t ); for ( ;; ) { - if ( !getCmdByte ) + if ( !getCmdByte ) { + if( pInput > pInputEnd ) + return 0; + cmdByte = *pInput++; } getCmdByte = ( getCmdByte + 1 ) & 0x07; if ( cmdByte & 0x01 ) { + if( pInput+1 > pInputEnd ) + return 0; + int position = *pInput++ << LZSS_LOOKSHIFT; position |= ( *pInput >> LZSS_LOOKSHIFT ); int count = ( *pInput++ & 0x0F ) + 1; - if ( count == 1 ) - { + if ( count == 1 ) break; - } + unsigned char *pSource = pOutput - position - 1; - if ( totalBytes + count > unBufSize ) - { + if ( totalBytes + count > unBufSize || + pSource < pOrigOutput ) return 0; - } for ( int i=0; i unBufSize ) + if ( totalBytes + 1 > unBufSize || + pInput > pInputEnd ) return 0; *pOutput++ = *pInput++; diff --git a/unittests/tier1test/lzsstest.cpp b/unittests/tier1test/lzsstest.cpp new file mode 100644 index 00000000..3e9d07ff --- /dev/null +++ b/unittests/tier1test/lzsstest.cpp @@ -0,0 +1,78 @@ +#include "tier0/dbg.h" +#include "unitlib/unitlib.h" +#include "tier1/lzss.h" + +#ifdef USING_ASAN +#include +#endif + +DEFINE_TESTSUITE( LZSSSafeUncompressTestSuite ) + +static void SafeUncompressTests() +{ + CLZSS lzss; + char poision1[8192]; + unsigned char in[256]; + + char poision2[8192]; + + unsigned char out[256]; + char poision3[8192]; + +#ifdef USING_ASAN + ASAN_POISON_MEMORY_REGION( poision1, 8192 ); + ASAN_POISON_MEMORY_REGION( poision2, 8192 ); + ASAN_POISON_MEMORY_REGION( poision3, 8192 ); +#endif + + lzss_header_t *pHeader = (lzss_header_t*)in; + pHeader->actualSize = sizeof(in)-sizeof(lzss_header_t); + pHeader->id = LZSS_ID; + + uint result = 0; + + // 0xff bytes test + memset(in+8, 0xFF, sizeof(in)-sizeof(lzss_header_t)); + result = lzss.SafeUncompress( in, sizeof(in), out, sizeof(out) ); + Shipping_Assert( result == 0 ); + + // zero bytes test + memset(in+8, 0x0, sizeof(in)-sizeof(lzss_header_t)); + result = lzss.SafeUncompress( in, sizeof(in), out, sizeof(out) ); + Shipping_Assert( result == 0 ); + + // valid data, invalid header + pHeader->actualSize = 1; + pHeader->id = LZSS_ID; + result = lzss.SafeUncompress( in, sizeof(in), out, sizeof(out) ); + Shipping_Assert( result == 0 ); + + pHeader->actualSize = 999; + pHeader->id = LZSS_ID; + result = lzss.SafeUncompress( in, sizeof(in), out, sizeof(out) ); + Shipping_Assert( result == 0 ); + + pHeader->actualSize = 999; + pHeader->id = 1337; + result = lzss.SafeUncompress( in, sizeof(in), out, sizeof(out) ); + Shipping_Assert( result == 0 ); + + + // valid header, valid data + const unsigned char compressed[] = {0x4c,0x5a,0x53,0x53,0x1a,0x0,0x0,0x0,0x0,0x44,0x6f,0x20,0x79,0x6f,0x75,0x20,0x6c,0x0,0x69,0x6b,0x65,0x20,0x77,0x68,0x61,0x74,0x41,0x0,0xd4,0x73,0x65,0x65,0x3f,0x0,0x0,0x0}; + + pHeader->actualSize = sizeof(compressed); + pHeader->id = LZSS_ID; + result = lzss.SafeUncompress( compressed, sizeof(compressed), out, sizeof(out) ); + + const char data[] = "Do you like what you see?"; + Shipping_Assert( memcmp(out, data, 26) == 0 ); + Shipping_Assert( result == 26 ); +} + +DEFINE_TESTCASE( LZSSSafeUncompressTest, LZSSSafeUncompressTestSuite ) +{ + Msg( "Running CLZSS::SafeUncompress tests\n" ); + + SafeUncompressTests(); +} diff --git a/unittests/tier1test/wscript b/unittests/tier1test/wscript index 774398ba..8421046f 100755 --- a/unittests/tier1test/wscript +++ b/unittests/tier1test/wscript @@ -14,7 +14,7 @@ def configure(conf): conf.define('TIER1TEST_EXPORTS', 1) def build(bld): - source = ['commandbuffertest.cpp', 'utlstringtest.cpp', 'tier1test.cpp'] #, 'lzsstest.cpp'] + source = ['commandbuffertest.cpp', 'utlstringtest.cpp', 'tier1test.cpp', 'lzsstest.cpp'] includes = ['../../public', '../../public/tier0'] defines = [] libs = ['tier0', 'tier1', 'mathlib', 'unitlib']