[FFmpeg-devel] [PATCH] avcodec/webp: Reinitilaize VP8 decoder on pixel format mismatch

[wm4]

On Wed, 10 May 2017 15:39:45 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Wed, May 10, 2017 at 08:10:23AM -0400, Ronald S. Bultje wrote:
> > Hi,
> > 
> > On Tue, May 9, 2017 at 9:24 PM, Michael Niedermayer <michael at niedermayer.cc>
> > wrote:
> >   
> > > On Tue, May 09, 2017 at 09:08:08PM -0400, Ronald S. Bultje wrote:  
> > > > Hi,
> > > >
> > > > On Tue, May 9, 2017 at 8:37 PM, Michael Niedermayer  
> > > <michael at niedermayer.cc>  
> > > > wrote:
> > > >  
> > > > > Fixes: out of array access
> > > > > Fixes: 1434/clusterfuzz-testcase-minimized-6314998085189632
> > > > > Fixes: 1435/clusterfuzz-testcase-minimized-6483783723253760
> > > > >
> > > > > Found-by: continuous fuzzing process https://github.com/google/oss-
> > > > > fuzz/tree/master/targets/ffmpeg
> > > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > > ---
> > > > >  libavcodec/webp.c | 9 +++++++--
> > > > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/libavcodec/webp.c b/libavcodec/webp.c
> > > > > index 16c3ae2662..23ed4bc26f 100644
> > > > > --- a/libavcodec/webp.c
> > > > > +++ b/libavcodec/webp.c
> > > > > @@ -1330,12 +1330,17 @@ static int vp8_lossy_decode_frame(  
> > > AVCodecContext  
> > > > > *avctx, AVFrame *p,
> > > > >      WebPContext *s = avctx->priv_data;
> > > > >      AVPacket pkt;
> > > > >      int ret;
> > > > > +    enum AVPixelFormat wanted_pix_fmt = s->has_alpha ?
> > > > > AV_PIX_FMT_YUVA420P : AV_PIX_FMT_YUV420P;
> > > > > +
> > > > > +    if (s->initialized && wanted_pix_fmt != avctx->pix_fmt) {
> > > > > +        ff_vp8_decode_free(avctx);
> > > > > +        s->initialized = 0;
> > > > > +    }
> > > > >
> > > > >      if (!s->initialized) {
> > > > >          ff_vp8_decode_init(avctx);
> > > > >          s->initialized = 1;
> > > > > -        if (s->has_alpha)
> > > > > -            avctx->pix_fmt = AV_PIX_FMT_YUVA420P;
> > > > > +        avctx->pix_fmt = wanted_pix_fmt;
> > > > >      }
> > > > >      s->lossless = 0;  
> > > >
> > > >
> > > > What is the out of array access? webp is intra only and the only thing  
> > > that  
> > > > is initialized with memory in that call is reference frames. What's going
> > > > on here?  
> > >
> > > webp uses the same context as VP8, and it changes the pixel format
> > > as it needs. Vp8 doesnt work if its format is changed under its feet  
> > 
> > 
> > I think what you're trying to say is that it doesn't work with RGBA as
> > pixel format (it shouldn't need a reset between yuv420p and yuv420pa). We
> > indeed don't set pix_fmt if has_alpha = 0, which seems like the core of the
> > issue. I'm not sure you need to call ff_vp8_decode_init() twice, in fact
> > I'm pretty sure you don't have to. It may also help to assert that pix_fmt
> > is yuv420p(a) when calling vp8 functions.  
> 
> The reason why i replied privatly is because the details of security
> issues should not be discussed in public before the fixes are available
> in releases. As these details are usefull not only to us understanding
> an issue but also to an attacker understanding and expoiting it
> 
> Ill reply privatly again to awnser your questions

Uh, just no.