[FFmpeg-devel] [PATCH] OpenEXR decoder rev-20
Michael Niedermayer
michaelni
Thu Apr 29 13:52:05 CEST 2010
On Sat, Apr 24, 2010 at 05:59:46PM +0200, Jimmy Christensen wrote:
[...]
>> [...]
>>> + // Process the lineOrder variable
>>> + if (check_header_variable(buf, buf_end, "lineOrder",
>>> "lineOrder", 25)) {
>>> + buf += 20;
>>> +
>>> + variable_buffer_data_size = get_header_variable_length(&buf,
>>> buf_end);
>>> + if (!variable_buffer_data_size)
>>> + return -1;
>>> + if (*buf) {
>>> + av_log(avctx, AV_LOG_ERROR, "Doesn't support this line
>>> order : %d\n", *buf);
>>> + return -1;
>>> + }
>>> +
>>> + buf += variable_buffer_data_size;
>>> + continue;
>>> + }
>>> +
>>> + // Process the compression variable
>>> + if (check_header_variable(buf, buf_end, "compression",
>>> "compression", 29)) {
>>
>>> + buf += 24;
>>> +
>>> + variable_buffer_data_size = get_header_variable_length(&buf,
>>> buf_end);
>>> + if (!variable_buffer_data_size)
>>> + return -1;
>>
>> that code is repeated all over the place
>>
>>
>
> Have moved the header jump to the check_header_variable function. which
> makes it a little less redundant. Hopefully enough.
>
>> [...]
>>> + if (w != avctx->width || h != avctx->height) {
>>> + // Verify the xmin, xmax, ymin, ymax and xdelta before setting
>>> the actual image size
>>> + if (xmin> w || xmin == ~0 ||
>>> + xmax> w || xmax == ~0 ||
>>> + ymin> h || ymin == ~0 ||
>>> + ymax> h || ymax == ~0 ||
>>
>> these look partly redundant
>>
>>
>
> Since these values are read from the file they could be wrong. Could
> perhaps move the check to earlier in the code.
they are partly redundant, remove the redundant checks.
also the new patch now is containing things that can be exploited that i
think where not in the previous
>
>>> + xdelta == ~0 ||
>>> + xdelta> avpkt->size / current_channel_offset) {
>>> + av_log(avctx, AV_LOG_ERROR, "Wrong sizing or missing size
>>> information\n");
>>> + return -1;
>>> + }
>>> + avcodec_set_dimensions(avctx, w, h);
>>> + }
>>> +
>>> + if (avctx->get_buffer(avctx, p)< 0) {
>>> + av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
>>> + return -1;
>>> + }
>>> +
>>> + ptr = p->data[0];
>>> + stride = p->linesize[0];
>>> +
>>> + // Zero out the start if ymin is not 0
>>> + for (y = 0; y< ymin; y++) {
>>> + memset(ptr, 0, avctx->width * 6);
>>> + ptr += stride;
>>> + }
>>> +
>>> + // Process the actual lines
>>> + for (y = ymin; y<= ymax; y++) {
>>> + uint16_t *ptr_x = (uint16_t *)ptr;
>>> + if (buf_end - buf> 8) {
>>> + const uint64_t line_offset = bytestream_get_le64(&buf) + 8;
>>> + // Check if the buffer has the required bytes needed from
>>> the offset
>>> + if (line_offset> avpkt->size - xdelta *
>>> current_channel_offset) {
>>
>> this code looks a little obfuscated, but at least its missing overflow
>> checks, also some critical checks further up are under ifs that if ever
>> false
>> would almost certainly be exploitable as the checks are no longer done ..
>>
>
> Have added more comments to explain the code. Could you point to which if
> statements you are talking about?
its half a year since i reviewed this, you dont expect me to remember
do you?
anyway, ive looked again and the first line i looked at looks exploitable
this code needs a complete review by the author against security issues
not fixing just the few i found.
also make sure you understand which sub expressions have which type in C
and at which point they overflow
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100429/19e7a253/attachment.pgp>
More information about the ffmpeg-devel
mailing list