[FFmpeg-cvslog] r15812 - in trunk/libavcodec: ac3dec.c ac3dec.h ac3dec_data.c ac3dec_data.h eac3dec.c

[Justin Ruggles]

Michael Niedermayer wrote:
> On Thu, Nov 13, 2008 at 05:42:53PM -0500, Justin Ruggles wrote:
>> Hi,
>>
>> Michael Niedermayer wrote:
>>> On Thu, Nov 13, 2008 at 04:18:13AM +0100, jbr wrote:
>>>> Author: jbr
>>>> Date: Thu Nov 13 04:18:13 2008
>>>> New Revision: 15812
>>>>
>>>> Log:
>>>> add support for spectral extension
>>> This code looks like it completely lacks validity checks and likely
>>> exploitable at several points.
>>> I am not asking you to revert it but i would be happy if you did anyway.
>>> This code should have passed review before commiting IMHO
>> I'm sorry.  I have reverted the appropriate files to r15811.
>>
>>> Below review is incomplete, there likely are more issues, also iam not
>>> mentioning the exploitable code as this patch needs to be reviewed completely
>>> for security issues (which i did not do) not just the one issue ive found
>>> fixed.
>> I'll make the suggested changes you have below, then submit a patch to
>> ffmpeg-devel.  Could you please let me know more information about the
>> expoitable parts of this code (off-list if you prefer)?
> 
> IIRC something along the lines of
> start=get_bits
> end= get_bits
> len= end - start    (end < start here)
> some code using len and really not expecting it to be <0
> 
> but as said the code needs a more throughout security review, theres a lot
> of reading and writing in arrays where the index depends on the bitstream.

I do see what you're referring to.  The code that does the same for
coupling has a check there.  Spectral extension ranges depend on 3
values from the bitstream: copy start subband, spx start subband, and
spx end subband.  I'll add checks to make sure copy_start < start < end.

Thanks,
Justin