CVE-2017-7585: Improper Restriction of Operations within the Bounds of a Memory Buffer (CWE-119)
value: In libsndfile before 1.0.28, an error in the "flac_buffer_copy()" function (flac.c) can be exploited to cause a stack-based buffer overflow via a specially crafted FLAC file.
CWE description: The product performs operations on a memory buffer, but it reads from or writes to a memory location outside the buffer's intended boundary. This may result in read or write operations on unexpected memory locations that could be linked to other variables, data structures, or internal program data.
CVSS v3 base score: 5.5
Published: 2017-04-07T20:59Z
Repository: https://github.com/erikd/libsndfile
Language: C
Files changed: src/flac.c
Commit date: 2017-04-02 18:13:30 +1000
Commit message: src/flac.c: Improve error handling

Especially when dealing with corrupt or malicious files.

Diff: diff --git a/src/flac.c b/src/flac.c
index 92029990..40629c7d 100644
--- a/src/flac.c
+++ b/src/flac.c
@@ -66,13 +66,13 @@ typedef struct
 
 	int32_t* encbuffer ;
 	unsigned bufferpos ;
 
 	const FLAC__Frame *frame ;
-	FLAC__bool bufferbackup ;
 
 	unsigned compression ;
+
 } FLAC_PRIVATE ;
 
 typedef struct
 {	const char *tag ;
 	int type ;
@@ -185,14 +185,13 @@ flac_buffer_copy (SF_PRIVATE *psf)
 
 	channels = SF_MIN (frame->header.channels, FLAC__MAX_CHANNELS) ;
 
 	if (pflac->ptr == NULL)
 	{	/*
-		**	Not sure why this code is here and not elsewhere.
-		**	Removing it causes valgrind errors.
+		** This pointer is reset to NULL each time the current frame has been
+		** decoded. Somehow its used during encoding and decoding.
 		*/
-		pflac->bufferbackup = SF_TRUE ;
 		for (i = 0 ; i < channels ; i++)
 		{
 			if (pflac->rbuffer [i] == NULL)
 				pflac->rbuffer [i] = calloc (FLAC__MAX_BLOCK_SIZE, sizeof (int32_t)) ;
 
@@ -204,10 +203,15 @@ flac_buffer_copy (SF_PRIVATE *psf)
 		} ;
 
 
 	len = SF_MIN (pflac->len, frame->header.blocksize) ;
 
+	if (pflac->remain % channels != 0)
+	{	psf_log_printf (psf, "Error: pflac->remain %u    channels %u\n", pflac->remain, channels) ;
+		return 0 ;
+		} ;
+
 	switch (pflac->pcmtype)
 	{	case PFLAC_PCM_SHORT :
 			{	short *retpcm = (short*) pflac->ptr ;
 				int shift = 16 - frame->header.bits_per_sample ;
 				if (shift < 0)
@@ -379,11 +383,10 @@ sf_flac_write_callback (const FLAC__StreamDecoder * UNUSED (decoder), const FLAC
 	FLAC_PRIVATE* pflac = (FLAC_PRIVATE*) psf->codec_data ;
 
 	pflac->frame = frame ;
 	pflac->bufferpos = 0 ;
 
-	pflac->bufferbackup = SF_FALSE ;
 	pflac->wbuffer = buffer ;
 
 	flac_buffer_copy (psf) ;
 
 	return FLAC__STREAM_DECODER_WRITE_STATUS_CONTINUE ;
@@ -905,25 +908,38 @@ flac_init (SF_PRIVATE *psf)
 } /* flac_init */
 
 static unsigned
 flac_read_loop (SF_PRIVATE *psf, unsigned len)
 {	FLAC_PRIVATE* pflac = (FLAC_PRIVATE*) psf->codec_data ;
+	FLAC__StreamDecoderState state ;
 
 	pflac->pos = 0 ;
 	pflac->len = len ;
 	pflac->remain = len ;
 
+	state = FLAC__stream_decoder_get_state (pflac->fsd) ;
+	if (state > FLAC__STREAM_DECODER_END_OF_STREAM)
+	{	psf_log_printf (psf, "FLAC__stream_decoder_get_state returned %s\n", FLAC__StreamDecoderStateString [state]) ;
+		/* Current frame is busted, so NULL the pointer. */
+		pflac->frame = NULL ;
+		} ;
+
 	/* First copy data that has already been decoded and buffered. */
 	if (pflac->frame != NULL && pflac->bufferpos < pflac->frame->header.blocksize)
 		flac_buffer_copy (psf) ;
 
 	/* Decode some more. */
 	while (pflac->pos < pflac->len)
 	{	if (FLAC__stream_decoder_process_single (pflac->fsd) == 0)
 			break ;
-		if (FLAC__stream_decoder_get_state (pflac->fsd) >= FLAC__STREAM_DECODER_END_OF_STREAM)
+		state = FLAC__stream_decoder_get_state (pflac->fsd) ;
+		if (state >= FLAC__STREAM_DECODER_END_OF_STREAM)
+		{	psf_log_printf (psf, "FLAC__stream_decoder_get_state returned %s\n", FLAC__StreamDecoderStateString [state]) ;
+			/* Current frame is busted, so NULL the pointer. */
+			pflac->frame = NULL ;
 			break ;
+			} ;
 		} ;
 
 	pflac->ptr = NULL ;
 
 	return pflac->pos ;

Vulnerable code: 	FLAC__bool bufferbackup ;
		**	Not sure why this code is here and not elsewhere.
		**	Removing it causes valgrind errors.
		pflac->bufferbackup = SF_TRUE ;
	pflac->bufferbackup = SF_FALSE ;
		if (FLAC__stream_decoder_get_state (pflac->fsd) >= FLAC__STREAM_DECODER_END_OF_STREAM)
Fixed code: 
		** This pointer is reset to NULL each time the current frame has been
		** decoded. Somehow its used during encoding and decoding.
	if (pflac->remain % channels != 0)
	{	psf_log_printf (psf, "Error: pflac->remain %u    channels %u\n", pflac->remain, channels) ;
		return 0 ;
		} ;

	FLAC__StreamDecoderState state ;
	state = FLAC__stream_decoder_get_state (pflac->fsd) ;
	if (state > FLAC__STREAM_DECODER_END_OF_STREAM)
	{	psf_log_printf (psf, "FLAC__stream_decoder_get_state returned %s\n", FLAC__StreamDecoderStateString [state]) ;
		/* Current frame is busted, so NULL the pointer. */
		pflac->frame = NULL ;
		} ;

		state = FLAC__stream_decoder_get_state (pflac->fsd) ;
		if (state >= FLAC__STREAM_DECODER_END_OF_STREAM)
		{	psf_log_printf (psf, "FLAC__stream_decoder_get_state returned %s\n", FLAC__StreamDecoderStateString [state]) ;
			/* Current frame is busted, so NULL the pointer. */
			pflac->frame = NULL ;
			} ;