[FFmpeg-devel] [PATCH] Fix uninitialized reads on malformed ogg files.

[Reimar Döffinger]

On 8 Mar 2012, at 22:46, Dale Curtis <dalecurtis at chromium.org> wrote:

> On Thu, Mar 8, 2012 at 1:40 PM, Reimar Döffinger
> <Reimar.Doeffinger at gmx.de>wrote:
> 
>> On Thu, Mar 08, 2012 at 12:27:35PM -0800, Dale Curtis wrote:
>>> On Thu, Mar 8, 2012 at 12:04 PM, Reimar Döffinger
>>> <Reimar.Doeffinger at gmx.de>wrote:
>>> 
>>>> On Thu, Mar 08, 2012 at 05:51:59AM +0100, Michael Niedermayer wrote:
>>>>> On Wed, Mar 07, 2012 at 02:26:58PM -0800, dalecurtis at chromium.orgwrote:
>>>>>> From: Dale Curtis <dalecurtis at chromium.org>
>>>>>> 
>>>>>> The ogg decoder wasn't padding the input buffer with the
>> appropriate
>>>>>> FF_INPUT_BUFFER_PADDING_SIZE bytes. Which led to uninitialized
>> reads in
>>>>>> various pieces of parsing code when they thought they had more data
>>>> than
>>>>>> they actually did.
>>>>> 
>>>>> patch looks good to me
>>>>> reimar ?
>>>> 
>>>> None of the code I am aware of is supposed to require padding, so
>>>> I'd really like to hear what code that is. Bugs tend to come in
>> bunches,
>>>> so I'd expect that code to be buggy in more ways.
>>>> I also have some doubts that that add can never cause integer
>> overflows.
>>>> 
>>> 
>>> The adds are so I could do the memset w/o using
>>> FFMIN(FF_INPUT_BUFFER_PADDING_SIZE, os->bufsize - os->bufpos). If you
>> don't
>>> think the buffers need padding, I'm happy to switch the patch around.
>>> 
>>> Specifically the code we found issue with was downstream in
>>> oggparsetheora.c. If there aren't enough bits left for header parsing,
>> the
>>> values will load uninitialized data:
>> 
>> Ok, I guess it is strictly seen necessary then.
>> However that code should at least be sanity-checking the buffer
>> size to be reasonable (e.g. the header is never less than around
>> 27 bytes, so we shouldn't even start to try anything smaller
>> than that).
>> That would probably already fix the case you were seeing,
>> even if it is still not fully correct.
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
> 
> That also fixes the issue and was the fix I discussed with Ronald prior to
> uploading this patch. The consensus was it's a little messy and seemed
> inconsistent with the padded buffer approach used elsewhere in FFmpeg.
> Here's what that code looks like:
> 
> // Ensure we have enough bits left to read the rest of the header.
> if (get_bits_left(&gb) < 149 ||
>    (thp->version >= 0x030200 && get_bits_left(&gb) < 149 + 102) ||
>    (thp->version >= 0x030400 && get_bits_left(&gb) < 149 + 102 + 100) ||
>    (thp->version >= 0x304000 && get_bits_left(&gb) < 149 + 102 + 100 + 2))
>    return -1;

My suggestion was to do the non-messy part, i.e. check the buffer size against one single value at the very start of the if block that starts the parsing.
That will result in a proper error in probably 99% of all randomly corrupted files.
The fact that it does not cover all cases means that your patch is necessary, but IMO this would be an improvement since it would give the user (and debugging developer) some hint what went wrong in most cases instead of setting values to 0 silently.