Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WavPack crashes -- SEGFAULT -- Invalid write #33

Closed
thuanpv opened this Issue Apr 23, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@thuanpv
Copy link

thuanpv commented Apr 23, 2018

Dear all,

This bug was found with AFLSmart, an extension of AFL. Thanks also to Marcel Böhme, Andrew Santosa and Alexandru Razvan Caciulescu. This could lead to denial of service and potentially code execution.

This bug was found on Ubuntu 16.04 64-bit & WavPack revision 0a7295 (HEAD)

To reproduce:
Download & extract the attached file - wavpack_crash5.wav
wavpack -y wavpack_crash5.wav

Error message:

 WAVPACK  Hybrid Lossless Audio Compressor  Linux Version 5.1.0
 Copyright (c) 1998 - 2017 David Bryant.  All Rights Reserved.

creating wavpack_crash5.wv,Segmentation fault

Valgrind says

creating wavpack_crash5.wv,==169803== Argument 'size' of function malloc has a fishy (possibly negative) value: -359324770
==169803==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==169803==    by 0x44AC47: ParseRiffHeaderConfig (riff.c:289)
==169803==    by 0x430762: pack_file (wavpack.c:1776)
==169803==    by 0x407442: main (wavpack.c:1272)
==169803== 
==169803== Invalid write of size 1
==169803==    at 0x4C3507C: __GI_mempcpy (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==169803==    by 0x51BC03D: _IO_file_xsgetn (fileops.c:1392)
==169803==    by 0x51B1235: fread (iofread.c:38)
==169803==    by 0x462496: fread (stdio2.h:295)
==169803==    by 0x462496: DoReadFile (utils.c:618)
==169803==    by 0x44ACE2: ParseRiffHeaderConfig (riff.c:296)
==169803==    by 0x430762: pack_file (wavpack.c:1776)
==169803==    by 0x407442: main (wavpack.c:1272)
==169803==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==169803== 
==169803== 
==169803== Process terminating with default action of signal 11 (SIGSEGV)
==169803==  Access not within mapped region at address 0x0
==169803==    at 0x4C3507C: __GI_mempcpy (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==169803==    by 0x51BC03D: _IO_file_xsgetn (fileops.c:1392)
==169803==    by 0x51B1235: fread (iofread.c:38)
==169803==    by 0x462496: fread (stdio2.h:295)
==169803==    by 0x462496: DoReadFile (utils.c:618)
==169803==    by 0x44ACE2: ParseRiffHeaderConfig (riff.c:296)
==169803==    by 0x430762: pack_file (wavpack.c:1776)
==169803==    by 0x407442: main (wavpack.c:1272)
==169803==  If you believe this happened as a result of a stack
==169803==  overflow in your program's main thread (unlikely but
==169803==  possible), you can try to increase the size of the
==169803==  main thread stack using the --main-stacksize= flag.
==169803==  The main thread stack size used in this run was 8388608.

ASAN says:

creating wavpack_crash5.wv,common_utils.c:626:104: runtime error: left shift of 234 by 24 places cannot be represented in type 'int'
==169891==WARNING: AddressSanitizer failed to allocate 0xffffffffea95239e bytes
==169891==AddressSanitizer's allocator is terminating the process instead of returning 0
==169891==If you don't like this behavior set allocator_may_return_null=1
==169891==AddressSanitizer CHECK failed: ../../../../src/libsanitizer/sanitizer_common/sanitizer_allocator.cc:145 "((0)) != (0)" (0x0, 0x0)
    #0 0x7f64437698ba  (/usr/lib/x86_64-linux-gnu/libasan.so.3+0xd08ba)
    #1 0x7f6443770283 in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) (/usr/lib/x86_64-linux-gnu/libasan.so.3+0xd7283)
    #2 0x7f644376e446  (/usr/lib/x86_64-linux-gnu/libasan.so.3+0xd5446)
    #3 0x7f64436be3b4  (/usr/lib/x86_64-linux-gnu/libasan.so.3+0x253b4)
    #4 0x7f644375ff34 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.3+0xc6f34)
    #5 0x41dcd8 in ParseRiffHeaderConfig /home/thuan/subjects/WavPack-asan/cli/riff.c:289
    #6 0x40cf26 in pack_file /home/thuan/subjects/WavPack-asan/cli/wavpack.c:1776
    #7 0x40a80b in main /home/thuan/subjects/WavPack-asan/cli/wavpack.c:1272
    #8 0x7f64422c782f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #9 0x402528 in _start (/home/thuan/subjects/WavPack-asan/cli/wavpack+0x402528)

Regards,

Thuan
wavpack_crash5.wav.tar.gz

@mboehme

This comment has been minimized.

Copy link

mboehme commented Apr 24, 2018

Here is our analysis:

A RIFF file starts out with a file header followed by a sequence of data chunks. A WAVE file is often just a RIFF file with a single "WAVE" chunk which consists of two sub-chunks -- a "fmt " chunk specifying the data format and a "data" chunk containing the actual sample data. http://soundfile.sapp.org/doc/WaveFormat/.

  1. We have the RIFF file header specify an unknown chunk (that is neither fmt, ds64, or data) with chunk size greater than 2^31
  2. WavPack enters the "unknown" chunk handling code in riff.c:286 which it just intends to copy to the output file
  3. With chunk size greater than 2^31, we have int bytes_to_copy = (chunk_header.ckSize + 1) & ~1L; in riff.c:288 overflow, such that bytes_to_copy is now negative (which valgrind flags).
  4. Malloc (riff.c:289) doesn't like negative parameters and overflows again to a much smaller positive number.
  5. Not enough memory is being allocated for the file that is to be read in DoReadFile in Line 296.
  6. WavPack finally SEGFAULTs in utils.c:618 at bcount = (uint32_t) fwrite ((unsigned char *) lpBuffer + *lpNumberOfBytesWritten, 1, nNumberOfBytesToWrite, hFile); where lpBuffer is our malloc'd buffer.

Suggested patch:

diff --git a/cli/riff.c b/cli/riff.c
index de98c1e..27ec298 100644
--- a/cli/riff.c
+++ b/cli/riff.c
@@ -286,6 +286,10 @@ int ParseRiffHeaderConfig (FILE *infile, char *infilename, char *fourcc, Wavpack
         else {          // just copy unknown chunks to output file
 
             int bytes_to_copy = (chunk_header.ckSize + 1) & ~1L;
+            if (bytes_to_copy < 0) {
+                error_line("%s has an unknown chunk that is too large for WavPack!", infilename);
+                return WAVPACK_SOFT_ERROR;
+            }
             char *buff = malloc (bytes_to_copy);
 
             if (debug_logging_mode)

@dbry dbry self-assigned this Apr 24, 2018

@dbry

This comment has been minimized.

Copy link
Owner

dbry commented Apr 24, 2018

Thanks for reporting this. I have already fixed the exact issue in caff.c and will apply it here.

dbry added a commit that referenced this issue Apr 25, 2018

@thuanpv

This comment has been minimized.

Copy link
Author

thuanpv commented Apr 26, 2018

The issue has been fixed! Thanks.

@thuanpv thuanpv closed this Apr 26, 2018

@dbry

This comment has been minimized.

Copy link
Owner

dbry commented Apr 26, 2018

Thanks for reporting these and thanks for the analysis!

@mboehme

This comment has been minimized.

Copy link

mboehme commented Apr 30, 2018

Thanks, David, for applying the patch so promptly.

For posteriority: MITRE assigned

  • CVE-2018-10538 for the WAVE-parser component (*.wav; ParseRiffHeaderConfig in riff.c)
  • CVE-2018-10539 for the DSDIFF-parser component (*.dsd; ParseWave64HeaderConfig in wave64.c).
  • CVE-2018-10540 for the WAVE64-parser component (*.w64; ParseDsdiffHeaderConfig in dsdiff.c).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.