Severe Remote Code Execution vulnerability #22

Activity

1. shoghicp commented on Apr 25, 2022

   

   shoghicp

   on Apr 25, 2022

   Last edited by shoghicp

   More actions

   After some short fuzzing sessions, they have definitely not been patched. The issues also run deeper and exist at many spots in the code (and additionally can cause double-free besides the out of bound read/writes).

   Gave up trying to fully patch this walking colander for personal projects, frameLength too small causes double-free on deletion of the decoder (add your own bounds check here if desired, still exploitable in other sections).

   Additionally, other places exist where bound checking is not done, and values coming from bitstream are fully trusted.

   Do note this patch is incomplete for other additional general issues found.

   diff --git a/codec/ALACBitUtilities.c b/codec/ALACBitUtilities.c
   index 9414889..92a2c21 100644
   --- a/codec/ALACBitUtilities.c
   +++ b/codec/ALACBitUtilities.c
   @@ -37,6 +37,8 @@ void BitBufferInit( BitBuffer * bits, uint8_t * buffer, uint32_t byteSize )
    	bits->byteSize	= byteSize;
    }
    
   +#define AssertBufferReadBounds(b, n) RequireAction( b->end >= (b->cur + ((b->bitIndex + n) / 8 + ((b->bitIndex + n) % 8 > 0))), return 0; );
   +
    // BitBufferRead
    //
    uint32_t BitBufferRead( BitBuffer * bits, uint8_t numBits )
   @@ -45,6 +47,8 @@ uint32_t BitBufferRead( BitBuffer * bits, uint8_t numBits )
    	
    	//Assert( numBits <= 16 );
    
   +	AssertBufferReadBounds(bits, numBits)
   +
    	returnBits = ((uint32_t)bits->cur[0] << 16) | ((uint32_t)bits->cur[1] << 8) | ((uint32_t)bits->cur[2]);
    	returnBits = returnBits << bits->bitIndex;
    	returnBits &= 0x00FFFFFF;
   @@ -55,7 +59,7 @@ uint32_t BitBufferRead( BitBuffer * bits, uint8_t numBits )
    	
    	bits->cur		+= (bits->bitIndex >> 3);
    	bits->bitIndex	&= 7;
   -	
   +
    	//Assert( bits->cur <= bits->end );
    	
    	return returnBits;
   @@ -69,6 +73,8 @@ uint8_t BitBufferReadSmall( BitBuffer * bits, uint8_t numBits )
    	uint16_t		returnBits;
    	
    	//Assert( numBits <= 8 );
   +
   +	AssertBufferReadBounds(bits, numBits)
    	
    	returnBits = (bits->cur[0] << 8) | bits->cur[1];
    	returnBits = returnBits << bits->bitIndex;
   @@ -92,6 +98,8 @@ uint8_t BitBufferReadOne( BitBuffer * bits )
    {
    	uint8_t		returnBits;
    
   +	AssertBufferReadBounds(bits, 8)
   +
    	returnBits = (bits->cur[0] >> (7 - bits->bitIndex)) & 1;
    
    	bits->bitIndex++;
   diff --git a/codec/ALACDecoder.cpp b/codec/ALACDecoder.cpp
   index ce3340d..c8f0d17 100644
   --- a/codec/ALACDecoder.cpp
   +++ b/codec/ALACDecoder.cpp
   @@ -128,8 +128,13 @@ int32_t ALACDecoder::Init( void * inMagicCookie, uint32_t inMagicCookieSize )
            theConfig.sampleRate = Swap32BtoN(((ALACSpecificConfig *)theActualCookie)->sampleRate);
    
            mConfig = theConfig;
   -        
   +
   +		//sanity checks
            RequireAction( mConfig.compatibleVersion <= kALACVersion, return kALAC_ParamError; );
   +		RequireAction( mConfig.bitDepth == 16 || mConfig.bitDepth == 20 || mConfig.bitDepth == 24 || mConfig.bitDepth == 32, return kALAC_ParamError; );
   +		RequireAction( mConfig.frameLength > 0 && mConfig.frameLength <= 16384, return kALAC_ParamError; );
   +		RequireAction( mConfig.sampleRate > 0 && mConfig.sampleRate <= 384000, return kALAC_ParamError; );
   +		RequireAction( mConfig.numChannels > 0 && mConfig.numChannels < kALACMaxChannels, return kALAC_ParamError; );
    
            // allocate mix buffers
            mMixBufferU = (int32_t *) calloc( mConfig.frameLength * sizeof(int32_t), 1 );
   @@ -251,6 +256,8 @@ int32_t ALACDecoder::Decode( BitBuffer * bits, uint8_t * sampleBuffer, uint32_t
    				{
    					numSamples  = BitBufferRead( bits, 16 ) << 16;
    					numSamples |= BitBufferRead( bits, 16 );
   +
   +					RequireAction( numSamples <= mConfig.frameLength, status = kALAC_ParamError; goto Exit; );
    				}
    
    				if ( escapeFlag == 0 )
   @@ -402,6 +409,8 @@ int32_t ALACDecoder::Decode( BitBuffer * bits, uint8_t * sampleBuffer, uint32_t
    				{
    					numSamples  = BitBufferRead( bits, 16 ) << 16;
    					numSamples |= BitBufferRead( bits, 16 );
   +
   +					RequireAction( numSamples <= mConfig.frameLength, status = kALAC_ParamError; goto Exit; );
    				}
    
    				if ( escapeFlag == 0 )

   👍React with 👍4Reacted by thisisted, Anjani Gourisaria, Anthony Parks and Georg
3. shoghicp commented on Apr 25, 2022

   

   shoghicp

   on Apr 25, 2022

   More actions

   They're not patched.

   I did a quick fuzz run ca. 20 hours ago and rediscovered CVE-2021-0674 CVE-2021-0675 almost instantly (few seconds)

   In my case found out of bounds read in ALACEncoder (possibly CVE-2021-0674) and out of bounds write in DecodeALAC (possibly CVE-2021-0675)

   In my testing, if frame sizes etc. are small enough but everything else is fine enough, ALACDecoder destructor will trigger a double-free but haven't checked further, afaik it's a corrupted allocation by an OOB write.
5. olivierlevon

   added a commit that references this issue on May 22, 2022

   Severe Remote Code Execution vulnerability macosforge#22

   ...

   eb3ae0c