======[ 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.
=======[4. Sample ]======
Please see the attached proof of concept file.
This has been assigned CVE-2017-8419
The most simple fix is indeed validating the number of channels before store.
I applied this fix in a branch of my lame 3.100a2 repository at https://github.com/eblanca/lame/tree/fix_num_channels .
Further suggestions are welcome.
Diff:
Thanks, the raised issues will be addressed in version 3.100.