[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