CVE-2017-8419: Multiple stack and heap corruptions from malicious file

--[ 1. Summary

Multiple security bugs exist in the Lame 3.99.5 MP3 encoder wherin a
malformed wav or aiff file can cause various stack or heap overflows.

--[ 2. Discussion

Despite the fact that the issue results in stack or heap overflows in
several locations, the original root cause is always the same. The root
cause is the use of signed integers for storing values read in from the
wav or aiff header of the source file. In particular, the num_channels
and thus the calculated num_samples variables.

The following is a snippet from parse_wave_header:

    ...
[1] 1355 int channels = 0;
    ...
    1382 format_tag = read_16_bits_low_high(sf);
    1383 subSize -= 2;
[2] 1384 channels = read_16_bits_low_high(sf);
    1385 subSize -= 2;
    ...
    1438 / make sure the header is sane /
[3] 1439 if (-1 == lame_set_num_channels(gfp, channels)) {
    1440 if (global_ui_config.silent < 10) {
    ...
[4] 1454 (void) lame_set_num_samples(gfp, data_length /
                (channels * ((bits_per_sample + 7) / 8)));
    1455 return 1;

As you can see, at [1] the number of channels is defined as a signed
integer which is read from the file at [2]. The read_16_bits_low_high
function is defined as returning a signed integer meaning the value can
be less than zero. Interestingly, this value is validated later in the
function at [3] in the lame_set_num_channels function. This is shown
below:

    ...
[5] 94 if (2 < num_channels || 0 == num_channels) {
    95 return -1; / we don't support more than 2 channels /
    96 }
    97 gfp->num_channels = num_channels;
    98 return 0;
    ...

As you can see at [5], the validity check just ensures that the number
of channels is not zero or greater than two; meaning a negative number
is considered valid and will pass this check. Then, later in the
parse_wave_header function, the number of samples (stored as an unsigned
integer) is calculated based on the size of the data, number of channels
and bits per sample. This causes an implicit signed to unsigned
conversion, meaning that num_samples is exceedingly large but considered
valid. Similar issues exist in the parsing of aiff file headers.
Clearly, this causes a number of issues throughout the processing of the
audio file as it will be expecting a significantly larger amount of data
than is available. Most notably a stack overflow in the get_audio_common
function. A snippet is shown below:

    ...
    730 static int
    731 get_audio_common(lame_t gfp, int buffer[2][1152],
                short buffer16[2][1152])
    732 {
    733    int     num_channels = lame_get_num_channels(gfp);
[6] 734 int insamp[2 * 1152];
    735 short buf_tmp16[2][1152];
    ...
    749
[7] 750 samples_to_read = framesize = lame_get_framesize(gfp);
    751 assert(framesize <= 1152);
    752
    ...
    799 samples_read =
[8] 800 read_samples_pcm(global.music_in, insamp, num_channels * samples_to_read);
    ...

An integer stack buffer is defined at [6] of a fixed size (2 * 1152).
This is the destination of the parsed audio data which is read using
read_samples_pcm at [8]. As the num_channels variable is negative and
the samples to read is 1152 the number of samples read is implicitly
converted to a very large size_t at the eventual call to fread. This
causes a very large read into a fixed size integer buffer causing a
stack smash.

--[ 3. Resolution

Only one exploit case is discussed here though testing discovered many
more. However, all of them would be prevented if the values parsed from
wav and aiff headers were correctly validated before use in the encoder.
Additionally, the use of signed integers is not advised when storing
buffer lengths.