#466 Signed Integer Overflow (65738191)

[Google-Autofuzz]

Hello flac team,

As part of our fuzzing efforts at Google, we have identified an issue affecting
flac (tested with revision * master 421961f00b505dcad41603305b47eda0b2ddfe92).

To reproduce, we are attaching a Dockerfile which compiles the project with
LLVM, taking advantage of the sanitizers that it offers. More information about
how to use the attached Dockerfile can be found here:
https://docs.docker.com/engine/reference/builder/

Instructions:
unzip artifacts_65738191.zip
docker build --build-arg SANITIZER=undefined --tag=autofuzz-flac-65738191 autofuzz_65738191
docker run --entrypoint /fuzzing/repro.sh --cap-add=SYS_PTRACE -v $PWD/autofuzz_65738191/poc-3aa012a1999dee3c9f817c022b2c79c89942f45d7b3309d744f69681aa2a6f40_min:/tmp/poc autofuzz-flac-65738191 "" /tmp/poc
docker run --cap-add=SYS_PTRACE -v $PWD/autofuzz_65738191/poc-3aa012a1999dee3c9f817c022b2c79c89942f45d7b3309d744f69681aa2a6f40_min:/tmp/poc -it autofuzz-flac-65738191

Alternatively, and depending on the bug, you could use gcc, valgrind or other
instrumentation tools to aid in the investigation. The sanitizer error that we
encountered is here:

sample rate    : 12000 Hz
channels       : 8
bits per sample: 16
total samples  : 53165582111
stream_decoder.c:2095:39: runtime error: signed integer overflow: 7995392 + 2142466002 cannot be represented in type 'int'
    #0 0x7f9167799695 in read_frame_ /fuzzing/flac/src/libFLAC/stream_decoder.c:2095:39
    #1 0x7f91677999ff in FLAC__stream_decoder_process_until_end_of_stream /fuzzing/flac/src/libFLAC/stream_decoder.c:1087:9
    #2 0x426136 in main /fuzzing/flac/examples/c/decode/file/main.c:101:8
    #3 0x7f91667ca2e0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202e0)
    #4 0x402e39 in _start (/fuzzing/flac/examples/c/decode/file/.libs/example_c_decode_file+0x402e39)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior stream_decoder.c:2095:39 in 

We will gladly work with you so you can successfully confirm and reproduce this
issue. Do let us know if you have any feedback surrounding the documentation.

Once you have reproduced the issue, we'd appreciate to learn your expected
timeline for an update to be released. With any fix, please attribute the report
to "Google Autofuzz project".

We are also pleased to inform you that your project is eligible for inclusion to
the OSS-Fuzz project, which can provide additional continuous fuzzing, and
encourage you to investigate integration options.

Don't hesitate to let us know if you have any questions!

Google AutoFuzz Team

[Erik]

I've reproduced it:

> examples/c/decode/file/example_c_decode_file autofuzz_65738191/poc-3aa012a1999dee3c9f817c022b2c79c89942f45d7b3309d744f69681aa2a6f40_min /dev/null
sample rate    : 12000 Hz
channels       : 8
bits per sample: 16
total samples  : 53165582111
stream_decoder.c:2095:39: runtime error: signed integer overflow: 7995392 + 2142466002 cannot be represented in type 'int'
ERROR: this example only supports 16bit stereo streams
decoding: FAILED
   state: FLAC__STREAM_DECODER_ABORTED

Thats the undefined behavior sanitizer being triggered.

The code for line 2095 is (wrapped):

     decoder->private_->output[0][i] += decoder->private_->output[1][i];

That code is the audio path. Undefined behavior on the audio path is not exploitable. Worst thing that can happen is bad sound.

A couple of years ago I spent at least a couple of hundred CPU hours fuzzing FLAC with the address sanitizer and fixed all the problems I found. I even spoke at a conference about it : https://www.youtube.com/watch?v=y0hyqzR6hIY

For an audio codec like FLAC I think fuzz testing with the undefined behaviour sanitizer is a waste of time, mainly because undefined behavior on the audio path is relatively harmless. Futhermore, undefined behavior in the logic will almost certainly result in a memory access that triggers the address sanitizer.

[Google-Autofuzz]

Thanks for taking a look at this issue!

If this does not represent any kind of problem, one thing to consider would be annotating the function with attributes to avoid sanitizer instrumentation:

http://releases.llvm.org/5.0.0/tools/clang/docs/UndefinedBehaviorSanitizer.html#disabling-instrumentation-with-attribute-no-sanitize-undefined

This may entail a small refactoring, i.e. to move 'safe' overflows that would result in garbage data into a dedicated function and keep other computations for things like array indexing/memory dereferencing in an instrumented function (if bugs like this are shallow/easy for fuzzers to find, that makes it more difficult to reach other parts of the code so annotations help in that way).

[Erik]

The code contains at least hundreds of these audio path only computations that may trigger undefined behavior. Refactoring them as you suggest would be a significant amount of work and likely have significant impact on the performance of the program.

My advice would be to only fuzz FLAC with the address santizer.