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

Submitted by Michael Niedermayer on May 10, 2017, 12:37 a.m.

Details

Message ID 20170510003722.30567-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer May 10, 2017, 12:37 a.m.
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@niedermayer.cc>
---
 libavcodec/webp.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Ronald S. Bultje May 10, 2017, 1:08 a.m.
Hi,

On Tue, May 9, 2017 at 8:37 PM, Michael Niedermayer <michael@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@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?

Ronald
Michael Niedermayer May 10, 2017, 1:24 a.m.
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@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@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

the reinit seemed reasonable cleanish to handle it. There are a few
other ways the same can be achived

Do you have a better idea or see something missing ?

thx

[...]
Ronald S. Bultje May 10, 2017, 2:10 a.m.
Hi,

On Tue, May 9, 2017 at 9:24 PM, Michael Niedermayer <michael@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@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@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
>
> the reinit seemed reasonable cleanish to handle it. There are a few
> other ways the same can be achived


What is the out of array access?

Ronald
Ronald S. Bultje May 10, 2017, 12:10 p.m.
Hi,

On Tue, May 9, 2017 at 9:24 PM, Michael Niedermayer <michael@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@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@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.

Ronald
Michael Niedermayer May 10, 2017, 1:39 p.m.
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@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@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@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

thx

[...]
wm4 May 11, 2017, 4:16 a.m.
On Wed, 10 May 2017 15:39:45 +0200
Michael Niedermayer <michael@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@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@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@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.

Patch hide | download patch | download mbox

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;