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

[Dale Curtis]

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;

- dale