Headline
CVE-2017-12562: Heap buffer overflows in `psf_binheader_writef` in 1.0.28 and later · Issue #292 · libsndfile/libsndfile
Heap-based Buffer Overflow in the psf_binheader_writef function in common.c in libsndfile through 1.0.28 allows remote attackers to cause a denial of service (application crash) or possibly have unspecified other impact.
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 © 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.