Incomplete fix for CVE-2018-19758

[ret2libc]

Hi,

I think the fix for CVE-2018-19758 in fa667c2 is not complete and it is still possible to trigger the same flaw just by adjusting the PoC a bit.

In sndfile.h.in:

	struct
	{	int mode ;
		uint32_t start ;
		uint32_t end ;
		uint32_t count ;
	} loops [16] ; /* make variable in a sensible way */

Loops is defined as an array of 16 elements, so reducing loop_count to a 16bit number does not really solve the out-of-bound read issue. The value should be checked and wrapped to 16 (not 16bits) or the process terminated immediately because the input is malformed.

How to reproduce

$ valgrind -- sndfile-convert ./incomplete-fix-CVE-2018-19758 a.wav
==5192== Memcheck, a memory error detector           
==5192== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. 
==5192== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==5192== Command: ./programs/sndfile-convert ../incomplete-fix-CVE-2018-19758 a.wav
==5192==                                            
==5192== Invalid read of size 4                               
==5192==    at 0x42C1BE: wav_write_header (wav.c:1154)
==5192==    by 0x409CE5: sf_writef_int (sndfile.c:2328)
==5192==    by 0x4032D2: sfe_copy_data_int (common.c:88)
==5192==    by 0x402F38: main (sndfile-convert.c:352) 
==5192==  Address 0x4bd7580 is 0 bytes after a block of size 272 alloc'd
==5192==    at 0x483AB1A: calloc (vg_replace_malloc.c:762)
==5192==    by 0x43D14D: psf_instrument_alloc (common.c:1293)
==5192==    by 0x406A6D: sf_command (sndfile.c:1309) 
==5192==    by 0x4030F8: copy_metadata (sndfile-convert.c:389)          
==5192==    by 0x402EE0: main (sndfile-convert.c:344)     
==5192==                                                     
==5192== Invalid read of size 4                     
==5192==    at 0x42C237: wav_write_header (wav.c:1158)        
==5192==    by 0x409CE5: sf_writef_int (sndfile.c:2328)
==5192==    by 0x4032D2: sfe_copy_data_int (common.c:88)
==5192==    by 0x402F38: main (sndfile-convert.c:352)
==5192==  Address 0x4bd7588 is 8 bytes after a block of size 272 alloc'd
==5192==    at 0x483AB1A: calloc (vg_replace_malloc.c:762)
==5192==    by 0x43D14D: psf_instrument_alloc (common.c:1293)
...

poc.tar.gz

Extra

If the issue is confirmed, I will request a new CVE for it as the old one may have been already fixed in some distributions, which would not get the real fix otherwise.

[erikd]

I can confirm that the existing fix is insufficient.

[ret2libc]

CVE-2019-3832 was assigned to this flaw.

[epozuelo]

Yeah, sounds like loop_count needs to be limited to 0xff. Looking at the code, I wonder if that's not the only problem though. In wav_read_smpl_chunk(), loop_count is read and its value is used to iterate over the loops array, without sanitizing it to [0-16] first. Thus if that functions deals with untrusted data, it can potentially read out of bounds.

[epozuelo]

Looks like wav_read_smpl_chunk is covered because of this check:

if (j < ARRAY_LEN (psf->instrument->loops))

I addressed wav_write_header in #460

[erikd]

#460 has been merged.