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

[Michael Niedermayer]

On Thu, Mar 08, 2012 at 09:04:51PM +0100, Reimar Döffinger 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.org wrote:
> > > 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

oggparse* uses get_bits(), ive not checked if they overread and if
the patch would fix it. It just looked plausible to me ...


> I'd really like to hear what code that is. Bugs tend tocome 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 only code that changes the buf size is
1. init to 65307
2. *= 2

if we assume that the existing code is free of overflows then the
largest value achivable is of the form of
65307 * 2^N where N is very likely 16 but could be 48 when int is
64bit
either way theres at least 15 million bytes from these to a overflow
FF_INPUT_BUFFER_PADDING_SIZE is smaller than 15 million

if the existing code can overflow i suspect it would crash with a
null pointer dereference before + FF_INPUT_BUFFER_PADDING_SIZE can
overflow

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120308/b83d4f6f/attachment.asc>