Leakage of uninitialized stack content in FIXED_POINT speex_decode()
The "speex.o" module provides several high level API functions for managing encoding and decoding procedures with speex. One of these functions is speex_decode, defined in libspeex/speex.c in line 115. It is a general purpose function for decoding data from a bit stream and placing them in an "out" buffer:
115: EXPORT int speex_decode(void *state, SpeexBits *bits, float *out)
{
int i, ret;
spx_int32_t N;
spx_int16_t short_out[MAX_IN_SAMPLES];
speex_decoder_ctl(state, SPEEX_GET_FRAME_SIZE, &N);
121: ret = (*((SpeexMode**)state))->dec(state, bits, short_out);
122:for (i=0;i<N;i++)
out[i] = short_out[i];
return ret;
}
It is worth noting that this definition of speex_decode is used in the "fixed point" implementation (libspeex/speex.c line 85).
The issue with this function is that if the call to (*((SpeexMode**)state))->dec at line 121 fails prematurely, the "for" loop at line 122 will copy the uninitialized "short_out" buffer directly into the output buffer (after performing a cast from a short to a float).
By examining the definition of the invoked structure-member function, one can see that it directly calls the decode function (see "dec") of the mode selected upon initialization (nb_decode/sb_decode):
include/speex.h:
/** Struct defining a Speex mode */
250:typedef struct SpeexMode {
/** Pointer to the low-level mode data */
const void *mode;
...
/** Pointer to frame decoding function */
283:decode_func dec;
/** ioctl-like requests for encoder */
encoder_ctl_func enc_ctl;
/** ioctl-like requests for decoder */
decoder_ctl_func dec_ctl;
} SpeexMode;
If the selected mode is narrowband, then the execution at libspeex/speex.c:121 will proceed at nb_decode, which is defined at libspeex/nb_celp.c:1333
1333: int nb_decode(void *state, SpeexBits *bits, void *vout)
{
...
spx_word16_t *out = (spx_word16_t*)vout;
...
do {
1379:if (speex_bits_remaining(bits)<5)
return -1;
...
m = speex_bits_unpack_unsigned(bits, 4);
...
1438:} else if (m>8) /* Invalid mode */
{
speex_notify("Invalid mode encountered. The stream is corrupted.");
return -2;
}
...
iir_mem16(st->exc, lpc, out, NB_FRAME_SIZE, NB_ORDER, st->mem_sp, stack);
...
}
Notice that there are several user-induced cases where the call to nb_decode (and hence to (*((SpeexMode**)state))->dec ) will fail before anything is written to the "vout" buffer. One case, at line 1379, is to provide an unexpectedly small bit stream. Another, at line 1438, is to provide an invalid mode.
Because of the fact that speex_decode is such a general purpose, high level function of the API, even simple programs that utilize the API are prone to trigger this bug. The following code is from sampledec.c which is a simple demo program that was included in the speex distribution as well as the speex manual:
int main(int argc, char **argv)
{
...
/*Create a new decoder state in narrowband mode*/
state = speex_decoder_init(&speex_nb_mode);
...
outFile = argv[1];
fout = fopen(outFile, "w");
/*Initialization of the structure that holds the bits*/
speex_bits_init(&bits);
while (1)
{
...
/*Read the "packet" encoded by sampleenc*/
fread(cbits, 1, nbBytes, stdin);
/*Copy the data into the bit-stream struct*/
speex_bits_read_from(&bits, cbits, nbBytes);
/*Decode the data*/
49:speex_decode(state, &bits, output);
/*Copy from float to short (16 bits) for output*/
52:for (i=0;i<FRAME_SIZE;i++)
out[i]=output[i];
/*Write the decoded audio to file*/
56:fwrite(out, sizeof(short), FRAME_SIZE, fout);
}
...
return 0;
}
One can observe that if a user supplies invalid data, that will cause the indirect call to nb_decode to fail prematurely and as explained, the output buffer will contain leaked stack content. Furthermore, this leaked content will be transported directly to the output (lines 52 - 56).
It is true that checking the return value of speex_decode would be sufficient to protect a program from using and further propagating the leaked stack content.
However we would like to point out two important factors here:
-
Firstly, the program linking to the speex library might have initialized the "out" buffer with zeroes so as to proactively prevent any buffer leakage to the output audio. The abovementioned bug in speex_decode would essentially nullify this proactive defense.
-
Secondly, there is a significant number of programs that use speex and which do not check for speex_decode's return value:
Therefore, we feel that the Speex code is sharing some of the responsibility for this stack content leak, and that is why we are raising this as a code quality issue on Speex rather than a Speex vulnerability.
A possible remediation would be to add return value checks immediately after the function call of line 121 at libspeex/speex.c. Alternatively, one could initialize the "short_out" buffer with zeroes prior to the dec() function call.
For a proof-of-concept please see the files in issue3_poc.tar.gz