Heap buffer overflows in `psf_binheader_writef` in 1.0.28 and later #292

manxorist · 2017-06-14T10:23:06Z

Case 's' only enlarges the buffer by 16 bytes instead of size bytes.
This issue had originally reported by funute against openmpt123 (see https://bugs.openmpt.org/view.php?id=974 ). openmpt123 uses libsndfile to write WAV files and can output large amounts of string data in the SF_STR_COMMENT metadata field.
Valgrind stacktrace (libsndfile 1.0.28):

manx@sagnix:~/projects/openmpt/trunk$ valgrind bin/openmpt123 --quiet test2.it --force -o test.wav
==23301== Memcheck, a memory error detector
==23301== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==23301== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==23301== Command: bin/openmpt123 --quiet test2.it --force -o test.wav
==23301== 
==23301== Invalid write of size 1
==23301==    at 0x4031F43: memcpy (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==23301==    by 0x4527B6E: memcpy (string3.h:53)
==23301==    by 0x4527B6E: psf_binheader_writef (common.c:687)
==23301==    by 0x450BC61: wavlike_write_strings (wavlike.c:1123)
==23301==    by 0x450C91C: wav_write_header (wav.c:1062)
==23301==    by 0x44EFCEC: sf_writef_float (sndfile.c:2451)
==23301==    by 0x805ACC4: openmpt123::sndfile_stream_raii::write(std::vector<float*, std::allocator<float*> >, unsigned int) (in /home/manx/projects/openmpt/trunk/bin/openmpt123)
[...]

Test case in bug1.c (tested on Ubuntu 14.04 x86-64 with libsndfile 1.0.28):

/* gcc -std=c99 -O2 -g -Wall -Wextra bug1.c -lsndfile && valgrind ./a.out */

#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <sndfile.h>

#define LENGTH_FRAMES 10

static int16_t buffer[LENGTH_FRAMES*2];

void add_string( SNDFILE * sf, int str_type, size_t len ) {

	char * str = NULL;

	str = calloc( len + 1, 1 );
	if ( !str ) {
		fprintf( stderr, "%s\n", "error" );
		exit( 1 );	
	}
	memset( str, 'x', len );
	sf_set_string( sf, str_type, str );
	free( str );
	str = NULL;

}

void test( int format, size_t first, size_t second, size_t third ) {

	SNDFILE * sf = NULL;
	SF_INFO info;
	memset( &info, 0, sizeof( SF_INFO ) );
	
	fprintf( stderr, "%zd %zd %zd\n", first, second, third );

	info.samplerate = 44100;
	info.channels = 2;
	info.format = format | SF_FORMAT_PCM_16;
	sf = sf_open( "dummy", SFM_WRITE, &info );
	if ( !sf ) {
		fprintf( stderr, "%s\n", "error" );
		exit( 1 );
	}
	
	add_string( sf, SF_STR_TITLE, first );
	add_string( sf, SF_STR_ARTIST, second );
	add_string( sf, SF_STR_COMMENT, third );
	
	sf_writef_short( sf, buffer, LENGTH_FRAMES );
	
	sf_close( sf );
	sf = NULL;

}

int main() {

	test( SF_FORMAT_WAV, 456, 190, 0 );

	/*
	for ( size_t a = 440; a <= 460; a++ ) {
		for ( size_t b = 190; b <= 210; b++ ) {
			for ( size_t c = 0; c <= 0; c++ ) {
				test( SF_FORMAT_WAV, a, b, c );
			}
		}
	}
	*/

	return 0;

}

manx@idefix ~/test $ gcc -std=c99 -O2 -g -Wall -Wextra bug1.c -lsndfile && valgrind ./a.out
==14278== Memcheck, a memory error detector
==14278== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==14278== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==14278== Command: ./a.out
==14278== 
456 190 0
==14278== Invalid write of size 2
==14278==    at 0x4C2F7E3: memcpy@@GLIBC_2.14 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14278==    by 0x4E6DD8F: memcpy (string3.h:51)
==14278==    by 0x4E6DD8F: psf_binheader_writef (common.c:687)
==14278==    by 0x4E56250: wavlike_write_strings (wavlike.c:1095)
==14278==    by 0x4E57001: wav_write_header (wav.c:1062)
==14278==    by 0x4E3F92D: sf_writef_short (sndfile.c:2223)
==14278==    by 0x400AFB: test (bug1.c:50)
==14278==    by 0x4008B9: main (bug1.c:59)
==14278==  Address 0x57791e0 is 0 bytes after a block of size 512 alloc'd
==14278==    at 0x4C2CE8E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14278==    by 0x4E6D923: psf_bump_header_allocation (common.c:68)
==14278==    by 0x4E6E8BC: psf_binheader_writef (common.c:680)
==14278==    by 0x4E56250: wavlike_write_strings (wavlike.c:1095)
==14278==    by 0x4E57001: wav_write_header (wav.c:1062)
==14278==    by 0x4E3F92D: sf_writef_short (sndfile.c:2223)
==14278==    by 0x400AFB: test (bug1.c:50)
==14278==    by 0x4008B9: main (bug1.c:59)
==14278== 
==14278== Invalid write of size 1
==14278==    at 0x4E6DDA8: psf_binheader_writef (common.c:689)
==14278==    by 0x4E56250: wavlike_write_strings (wavlike.c:1095)
==14278==    by 0x4E57001: wav_write_header (wav.c:1062)
==14278==    by 0x4E3F92D: sf_writef_short (sndfile.c:2223)
==14278==    by 0x400AFB: test (bug1.c:50)
==14278==    by 0x4008B9: main (bug1.c:59)
==14278==  Address 0x57791e1 is 1 bytes after a block of size 512 alloc'd
==14278==    at 0x4C2CE8E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14278==    by 0x4E6D923: psf_bump_header_allocation (common.c:68)
==14278==    by 0x4E6E8BC: psf_binheader_writef (common.c:680)
==14278==    by 0x4E56250: wavlike_write_strings (wavlike.c:1095)
==14278==    by 0x4E57001: wav_write_header (wav.c:1062)
==14278==    by 0x4E3F92D: sf_writef_short (sndfile.c:2223)
==14278==    by 0x400AFB: test (bug1.c:50)
==14278==    by 0x4008B9: main (bug1.c:59)
==14278== 
==14278== Syscall param write(buf) points to uninitialised byte(s)
==14278==    at 0x5195F80: __write_nocancel (syscall-template.S:81)
==14278==    by 0x4E712C9: psf_fwrite (file_io.c:377)
==14278==    by 0x4E56C3E: wav_write_header (wav.c:1120)
==14278==    by 0x4E3F92D: sf_writef_short (sndfile.c:2223)
==14278==    by 0x400AFB: test (bug1.c:50)
==14278==    by 0x4008B9: main (bug1.c:59)
==14278==  Address 0x5779420 is 512 bytes inside a block of size 1,024 alloc'd
==14278==    at 0x4C2CE8E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14278==    by 0x4E6D923: psf_bump_header_allocation (common.c:68)
==14278==    by 0x4E6E63C: psf_binheader_writef (common.c:563)
==14278==    by 0x4E561C0: wavlike_write_strings (wavlike.c:1103)
==14278==    by 0x4E57001: wav_write_header (wav.c:1062)
==14278==    by 0x4E3F92D: sf_writef_short (sndfile.c:2223)
==14278==    by 0x400AFB: test (bug1.c:50)
==14278==    by 0x4008B9: main (bug1.c:59)
==14278== 
==14278== 
==14278== HEAP SUMMARY:
==14278==     in use at exit: 0 bytes in 0 blocks
==14278==   total heap usage: 10 allocs, 10 frees, 17,271 bytes allocated
==14278== 
==14278== All heap blocks were freed -- no leaks are possible
==14278== 
==14278== For counts of detected and suppressed errors, rerun with: -v
==14278== Use --track-origins=yes to see where uninitialised values come from
==14278== ERROR SUMMARY: 4 errors from 3 contexts (suppressed: 0 from 0)

psf_binheader_writef in src/common.c enlarges the header buffer (if needed) prior to the big switch statement by an amount (16 bytes) which is enough for all cases where only a single value gets added. Cases 's', 'S', 'p' however additionally write an arbitrary length block of data and again enlarge the buffer to the required amount. However, the required space calculation does not take into account the size of the length field which gets output before the data.
Test case bug2.c (tested on Ubuntu 14.04 x86-64 with libsndfile 1.0.28 with 16 replaced by size in the buffer size calculation):

/* gcc -std=c99 -O2 -g -Wall -Wextra bug2.c -lsndfile && valgrind ./a.out */

#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <sndfile.h>

#define LENGTH_FRAMES 10

static int16_t buffer[LENGTH_FRAMES*2];

void add_string( SNDFILE * sf, int str_type, size_t len ) {

	char * str = NULL;

	str = calloc( len + 1, 1 );
	if ( !str ) {
		fprintf( stderr, "%s\n", "error" );
		exit( 1 );	
	}
	memset( str, 'x', len );
	sf_set_string( sf, str_type, str );
	free( str );
	str = NULL;

}

void test( int format, size_t first, size_t second, size_t third ) {

	SNDFILE * sf = NULL;
	SF_INFO info;
	memset( &info, 0, sizeof( SF_INFO ) );

	fprintf( stderr, "%zd %zd %zd\n", first, second, third );
	
	info.samplerate = 44100;
	info.channels = 2;
	info.format = format | SF_FORMAT_PCM_16;
	sf = sf_open( "dummy", SFM_WRITE, &info );
	if ( !sf ) {
		fprintf( stderr, "%s\n", "error" );
		exit( 1 );
	}
	
	add_string( sf, SF_STR_TITLE, first );
	add_string( sf, SF_STR_ARTIST, second );
	add_string( sf, SF_STR_COMMENT, third );
	
	sf_writef_short( sf, buffer, LENGTH_FRAMES );
	
	sf_close( sf );
	sf = NULL;

}

int main() {

	test( SF_FORMAT_WAV, 0, 200, 0 );

	/*
	for ( size_t a = 0; a <= 20; a++ ) {
		for ( size_t b = 190; b <= 210; b++ ) {
			for ( size_t c = 0; c <= 0; c++ ) {
				test( SF_FORMAT_WAV, a, b, c );
			}
		}
	}
	*/

	return 0;

}

manx@idefix ~/test $ gcc -std=c99 -O2 -g -Wall -Wextra bug2.c -lsndfile && valgrind ./a.out
==15481== Memcheck, a memory error detector
==15481== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==15481== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==15481== Command: ./a.out
==15481== 
0 200 0
==15481== Invalid write of size 2
==15481==    at 0x4C2F7E3: memcpy@@GLIBC_2.14 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==15481==    by 0x4E6DD8F: memcpy (string3.h:51)
==15481==    by 0x4E6DD8F: psf_binheader_writef (common.c:687)
==15481==    by 0x4E561C0: wavlike_write_strings (wavlike.c:1103)
==15481==    by 0x4E57001: wav_write_header (wav.c:1062)
==15481==    by 0x4E3F92D: sf_writef_short (sndfile.c:2223)
==15481==    by 0x400AFB: test (bug2.c:50)
==15481==    by 0x4008B6: main (bug2.c:59)
==15481==  Address 0x5778340 is 0 bytes after a block of size 256 alloc'd
==15481==    at 0x4C2CC70: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==15481==    by 0x4E6CBF6: psf_allocate (common.c:46)
==15481==    by 0x4E41437: sf_open (sndfile.c:330)
==15481==    by 0x400AB1: test (bug2.c:40)
==15481==    by 0x4008B6: main (bug2.c:59)
==15481== 
==15481== Invalid write of size 1
==15481==    at 0x4E6DDA8: psf_binheader_writef (common.c:689)
==15481==    by 0x4E561C0: wavlike_write_strings (wavlike.c:1103)
==15481==    by 0x4E57001: wav_write_header (wav.c:1062)
==15481==    by 0x4E3F92D: sf_writef_short (sndfile.c:2223)
==15481==    by 0x400AFB: test (bug2.c:50)
==15481==    by 0x4008B6: main (bug2.c:59)
==15481==  Address 0x5778341 is 1 bytes after a block of size 256 alloc'd
==15481==    at 0x4C2CC70: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==15481==    by 0x4E6CBF6: psf_allocate (common.c:46)
==15481==    by 0x4E41437: sf_open (sndfile.c:330)
==15481==    by 0x400AB1: test (bug2.c:40)
==15481==    by 0x4008B6: main (bug2.c:59)
==15481== 
==15481== Syscall param write(buf) points to uninitialised byte(s)
==15481==    at 0x5195F80: __write_nocancel (syscall-template.S:81)
==15481==    by 0x4E712B9: psf_fwrite (file_io.c:377)
==15481==    by 0x4E56C3E: wav_write_header (wav.c:1120)
==15481==    by 0x4E3F92D: sf_writef_short (sndfile.c:2223)
==15481==    by 0x400AFB: test (bug2.c:50)
==15481==    by 0x4008B6: main (bug2.c:59)
==15481==  Address 0x57789c0 is 256 bytes inside a block of size 512 alloc'd
==15481==    at 0x4C2CE8E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==15481==    by 0x4E6D923: psf_bump_header_allocation (common.c:68)
==15481==    by 0x4E6E63C: psf_binheader_writef (common.c:563)
==15481==    by 0x4E56C23: wav_write_header (wav.c:1119)
==15481==    by 0x4E3F92D: sf_writef_short (sndfile.c:2223)
==15481==    by 0x400AFB: test (bug2.c:50)
==15481==    by 0x4008B6: main (bug2.c:59)
==15481== 
==15481== 
==15481== HEAP SUMMARY:
==15481==     in use at exit: 0 bytes in 0 blocks
==15481==   total heap usage: 8 allocs, 8 frees, 14,491 bytes allocated
==15481== 
==15481== All heap blocks were freed -- no leaks are possible
==15481== 
==15481== For counts of detected and suppressed errors, rerun with: -v
==15481== Use --track-origins=yes to see where uninitialised values come from
==15481== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)
manx@idefix ~/test $

Buffer size requirement calculation in case 'S' does not account for the padding byte (size += (size & 1) ; happens after the calculation which uses size).
Case 'S' can overrun the header buffer by 1 byte when no padding is involved (memcpy (&(psf->header.ptr [psf->header.indx]), strptr, size + 1) ; while the buffer is only guaranteed to have size space available).
psf->header.ptr [psf->header.indx] = 0 ; in case 'S' always writes 1 byte beyond the space which is guaranteed to be allocated in the header buffer.
Case 's' can overrun the provided source string by 1 byte if padding is involved (memcpy (&(psf->header.ptr [psf->header.indx]), strptr, size) ; where size is strlen (strptr) + 1 (which includes the 0 terminator, plus optionally another 1 which is padding and not guaranteed to be readable via the source string pointer).

I have not yet invested the time to provide reproducable tests for 3, 4, 5, 6:

3 to 5 require hitting the header buffer end exactly which might not even be possible with 2 byte alignment and the buffer enlargment being also 2 byte aligned (I did not investigate this further, triggering might require an odd INITAL_HEADER_SIZE constant and/or a growing factor other than 2). I would consider the current code to at least be fragile nonetheless.
6 is difficult to even catch with valgrind or similar tools because the source strings are layed out in one big buffer contiguously (psf->strings.storage) which makes it impossible for memory checkers to detect buffer overruns inside that buffer.

Pull request with fixes will follow.

The text was updated successfully, but these errors were encountered:

…ader Fixes the following problems: 1. Case 's' only enlarges the buffer by 16 bytes instead of size bytes. 2. psf_binheader_writef() enlarges the header buffer (if needed) prior to the big switch statement by an amount (16 bytes) which is enough for all cases where only a single value gets added. Cases 's', 'S', 'p' however additionally write an arbitrary length block of data and again enlarge the buffer to the required amount. However, the required space calculation does not take into account the size of the length field which gets output before the data. 3. Buffer size requirement calculation in case 'S' does not account for the padding byte ("size += (size & 1) ;" happens after the calculation which uses "size"). 4. Case 'S' can overrun the header buffer by 1 byte when no padding is involved ("memcpy (&(psf->header.ptr [psf->header.indx]), strptr, size + 1) ;" while the buffer is only guaranteed to have "size" space available). 5. "psf->header.ptr [psf->header.indx] = 0 ;" in case 'S' always writes 1 byte beyond the space which is guaranteed to be allocated in the header buffer. 6. Case 's' can overrun the provided source string by 1 byte if padding is involved ("memcpy (&(psf->header.ptr [psf->header.indx]), strptr, size) ;" where "size" is "strlen (strptr) + 1" (which includes the 0 terminator, plus optionally another 1 which is padding and not guaranteed to be readable via the source string pointer). Closes: libsndfile#292

…ader Fixes the following problems: 1. Case 's' only enlarges the buffer by 16 bytes instead of size bytes. 2. psf_binheader_writef() enlarges the header buffer (if needed) prior to the big switch statement by an amount (16 bytes) which is enough for all cases where only a single value gets added. Cases 's', 'S', 'p' however additionally write an arbitrary length block of data and again enlarge the buffer to the required amount. However, the required space calculation does not take into account the size of the length field which gets output before the data. 3. Buffer size requirement calculation in case 'S' does not account for the padding byte ("size += (size & 1) ;" happens after the calculation which uses "size"). 4. Case 'S' can overrun the header buffer by 1 byte when no padding is involved ("memcpy (&(psf->header.ptr [psf->header.indx]), strptr, size + 1) ;" while the buffer is only guaranteed to have "size" space available). 5. "psf->header.ptr [psf->header.indx] = 0 ;" in case 'S' always writes 1 byte beyond the space which is guaranteed to be allocated in the header buffer. 6. Case 's' can overrun the provided source string by 1 byte if padding is involved ("memcpy (&(psf->header.ptr [psf->header.indx]), strptr, size) ;" where "size" is "strlen (strptr) + 1" (which includes the 0 terminator, plus optionally another 1 which is padding and not guaranteed to be readable via the source string pointer). Closes: #292

agx · 2017-07-21T09:36:29Z

Hi @erikd
will there be CVE request for this one or has this already happened (want to avoid duplication)

erikd · 2017-07-28T22:45:56Z

I have not requested one. No idea if anyone else has.

manxorist · 2017-07-29T06:08:30Z

I did not request a CVE either.

agx · 2017-08-04T16:24:20Z

O.k., I've asked for a CVE now

attritionorg · 2017-08-05T17:54:46Z

CVE-2017-12562

mrkz · 2017-08-10T15:17:42Z

Thanks for requesting the CVE. @erikd Any chance to get a minor release with this fix?

erikd · 2017-08-10T19:12:40Z

I'll try to do one this weekend.

ctheune · 2017-10-11T13:14:42Z

Not wanting to be nosy, but AFAICT that minor release didn't happen, right? Any chance to get one?

Bombe · 2020-04-28T15:21:47Z

[poking noises] anybody alive in here? A release with this bugfix would be pretty sweet…

manxorist mentioned this issue Jun 14, 2017

src/common.c: Fix heap buffer overflows when writing strings in binheader #293

Merged

erikd closed this as completed in #293 Jun 15, 2017

manxorist mentioned this issue Nov 23, 2017

Crash under ARM when closing file after writing a large SF_STR_COMMENT #339

Closed

This was referenced Jul 11, 2020

libsndfile 1.0.28 contains multiple security bugs Homebrew/homebrew-core#57833

Closed

1.0.29 release plan? #470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Heap buffer overflows in `psf_binheader_writef` in 1.0.28 and later #292

Heap buffer overflows in `psf_binheader_writef` in 1.0.28 and later #292

manxorist commented Jun 14, 2017 •

edited

Loading

agx commented Jul 21, 2017

erikd commented Jul 28, 2017

manxorist commented Jul 29, 2017

agx commented Aug 4, 2017

attritionorg commented Aug 5, 2017

mrkz commented Aug 10, 2017

erikd commented Aug 10, 2017

ctheune commented Oct 11, 2017

Bombe commented Apr 28, 2020

Heap buffer overflows in psf_binheader_writef in 1.0.28 and later #292

Heap buffer overflows in psf_binheader_writef in 1.0.28 and later #292

Comments

manxorist commented Jun 14, 2017 • edited Loading

agx commented Jul 21, 2017

erikd commented Jul 28, 2017

manxorist commented Jul 29, 2017

agx commented Aug 4, 2017

attritionorg commented Aug 5, 2017

mrkz commented Aug 10, 2017

erikd commented Aug 10, 2017

ctheune commented Oct 11, 2017

Bombe commented Apr 28, 2020

Heap buffer overflows in `psf_binheader_writef` in 1.0.28 and later #292

Heap buffer overflows in `psf_binheader_writef` in 1.0.28 and later #292

manxorist commented Jun 14, 2017 •

edited

Loading