diff mbox series

[FFmpeg-devel] avcodec/mobiclip: Check quantizer before table setup

Message ID 20200910231959.15573-1-michael@niedermayer.cc
State Accepted
Commit bad8b17a3da219777341acafd3e3113ea2477484
Headers show
Series [FFmpeg-devel] avcodec/mobiclip: Check quantizer before table setup
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Michael Niedermayer Sept. 10, 2020, 11:19 p.m. UTC
Fixes: index -1 out of bounds for type 'const uint8_t [6][16]'
Fixes: out of array read
Fixes: shift exponent -21 is negative
Fixes: 25422/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MOBICLIP_fuzzer-5748258226569216
Fixes: shift exponent 8039082 is too large for 32-bit type 'int'
Fixes: 25430/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MOBICLIP_fuzzer-5698567770210304

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/mobiclip.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Paul B Mahol Sept. 11, 2020, 9:12 a.m. UTC | #1
On Fri, Sep 11, 2020 at 01:19:59AM +0200, Michael Niedermayer wrote:
> Fixes: index -1 out of bounds for type 'const uint8_t [6][16]'
> Fixes: out of array read
> Fixes: shift exponent -21 is negative
> Fixes: 25422/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MOBICLIP_fuzzer-5748258226569216
> Fixes: shift exponent 8039082 is too large for 32-bit type 'int'
> Fixes: 25430/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MOBICLIP_fuzzer-5698567770210304
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/mobiclip.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/mobiclip.c b/libavcodec/mobiclip.c
> index d147eddbae..13f0edc89d 100644
> --- a/libavcodec/mobiclip.c
> +++ b/libavcodec/mobiclip.c
> @@ -1323,6 +1323,10 @@ static int mobiclip_decode(AVCodecContext *avctx, void *data,
>          }
>      } else {
>          MotionXY *motion = s->motion;
> +        int quantizer = s->quantizer + get_se_golomb(gb);
> +
> +        if (quantizer < 12 || quantizer > 161)

From where this numbers come?
I think this is too aggressive.

> +            return AVERROR_INVALIDDATA;
>  
>          memset(motion, 0, s->motion_size);
>  
> @@ -1330,7 +1334,7 @@ static int mobiclip_decode(AVCodecContext *avctx, void *data,
>          frame->key_frame = 0;
>          s->dct_tab_idx = 0;
>  
> -        setup_qtables(avctx, s->quantizer + get_se_golomb(gb));
> +        setup_qtables(avctx, quantizer);
>          for (int y = 0; y < avctx->height; y += 16) {
>              for (int x = 0; x < avctx->width; x += 16) {
>                  int idx;
> -- 
> 2.17.1
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer Sept. 11, 2020, 6:09 p.m. UTC | #2
On Fri, Sep 11, 2020 at 11:12:33AM +0200, Paul B Mahol wrote:
> On Fri, Sep 11, 2020 at 01:19:59AM +0200, Michael Niedermayer wrote:
> > Fixes: index -1 out of bounds for type 'const uint8_t [6][16]'
> > Fixes: out of array read
> > Fixes: shift exponent -21 is negative
> > Fixes: 25422/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MOBICLIP_fuzzer-5748258226569216
> > Fixes: shift exponent 8039082 is too large for 32-bit type 'int'
> > Fixes: 25430/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MOBICLIP_fuzzer-5698567770210304
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/mobiclip.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/mobiclip.c b/libavcodec/mobiclip.c
> > index d147eddbae..13f0edc89d 100644
> > --- a/libavcodec/mobiclip.c
> > +++ b/libavcodec/mobiclip.c
> > @@ -1323,6 +1323,10 @@ static int mobiclip_decode(AVCodecContext *avctx, void *data,
> >          }
> >      } else {
> >          MotionXY *motion = s->motion;
> > +        int quantizer = s->quantizer + get_se_golomb(gb);
> > +
> > +        if (quantizer < 12 || quantizer > 161)
> 
> From where this numbers come?

It comes from the implementation
qtab is int and will overflow beyond 161

and below 12 the shift will be negative here:
s->qtab[1][i] = quant8x8_tab[qx][i] << (qy - 2);


> I think this is too aggressive.


[...]
Paul B Mahol Sept. 11, 2020, 8 p.m. UTC | #3
On Fri, Sep 11, 2020 at 08:09:48PM +0200, Michael Niedermayer wrote:
> On Fri, Sep 11, 2020 at 11:12:33AM +0200, Paul B Mahol wrote:
> > On Fri, Sep 11, 2020 at 01:19:59AM +0200, Michael Niedermayer wrote:
> > > Fixes: index -1 out of bounds for type 'const uint8_t [6][16]'
> > > Fixes: out of array read
> > > Fixes: shift exponent -21 is negative
> > > Fixes: 25422/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MOBICLIP_fuzzer-5748258226569216
> > > Fixes: shift exponent 8039082 is too large for 32-bit type 'int'
> > > Fixes: 25430/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MOBICLIP_fuzzer-5698567770210304
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavcodec/mobiclip.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/libavcodec/mobiclip.c b/libavcodec/mobiclip.c
> > > index d147eddbae..13f0edc89d 100644
> > > --- a/libavcodec/mobiclip.c
> > > +++ b/libavcodec/mobiclip.c
> > > @@ -1323,6 +1323,10 @@ static int mobiclip_decode(AVCodecContext *avctx, void *data,
> > >          }
> > >      } else {
> > >          MotionXY *motion = s->motion;
> > > +        int quantizer = s->quantizer + get_se_golomb(gb);
> > > +
> > > +        if (quantizer < 12 || quantizer > 161)
> > 
> > From where this numbers come?
> 
> It comes from the implementation
> qtab is int and will overflow beyond 161

ok then.

> 
> and below 12 the shift will be negative here:
> s->qtab[1][i] = quant8x8_tab[qx][i] << (qy - 2);
> 
> 
> > I think this is too aggressive.
> 
> 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Many that live deserve death. And some that die deserve life. Can you give
> it to them? Then do not be too eager to deal out death in judgement. For
> even the very wise cannot see all ends. -- Gandalf



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer Sept. 12, 2020, 12:15 p.m. UTC | #4
On Fri, Sep 11, 2020 at 10:00:02PM +0200, Paul B Mahol wrote:
> On Fri, Sep 11, 2020 at 08:09:48PM +0200, Michael Niedermayer wrote:
> > On Fri, Sep 11, 2020 at 11:12:33AM +0200, Paul B Mahol wrote:
> > > On Fri, Sep 11, 2020 at 01:19:59AM +0200, Michael Niedermayer wrote:
> > > > Fixes: index -1 out of bounds for type 'const uint8_t [6][16]'
> > > > Fixes: out of array read
> > > > Fixes: shift exponent -21 is negative
> > > > Fixes: 25422/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MOBICLIP_fuzzer-5748258226569216
> > > > Fixes: shift exponent 8039082 is too large for 32-bit type 'int'
> > > > Fixes: 25430/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MOBICLIP_fuzzer-5698567770210304
> > > > 
> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  libavcodec/mobiclip.c | 6 +++++-
> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/libavcodec/mobiclip.c b/libavcodec/mobiclip.c
> > > > index d147eddbae..13f0edc89d 100644
> > > > --- a/libavcodec/mobiclip.c
> > > > +++ b/libavcodec/mobiclip.c
> > > > @@ -1323,6 +1323,10 @@ static int mobiclip_decode(AVCodecContext *avctx, void *data,
> > > >          }
> > > >      } else {
> > > >          MotionXY *motion = s->motion;
> > > > +        int quantizer = s->quantizer + get_se_golomb(gb);
> > > > +
> > > > +        if (quantizer < 12 || quantizer > 161)
> > > 
> > > From where this numbers come?
> > 
> > It comes from the implementation
> > qtab is int and will overflow beyond 161
> 
> ok then.

will apply

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/mobiclip.c b/libavcodec/mobiclip.c
index d147eddbae..13f0edc89d 100644
--- a/libavcodec/mobiclip.c
+++ b/libavcodec/mobiclip.c
@@ -1323,6 +1323,10 @@  static int mobiclip_decode(AVCodecContext *avctx, void *data,
         }
     } else {
         MotionXY *motion = s->motion;
+        int quantizer = s->quantizer + get_se_golomb(gb);
+
+        if (quantizer < 12 || quantizer > 161)
+            return AVERROR_INVALIDDATA;
 
         memset(motion, 0, s->motion_size);
 
@@ -1330,7 +1334,7 @@  static int mobiclip_decode(AVCodecContext *avctx, void *data,
         frame->key_frame = 0;
         s->dct_tab_idx = 0;
 
-        setup_qtables(avctx, s->quantizer + get_se_golomb(gb));
+        setup_qtables(avctx, quantizer);
         for (int y = 0; y < avctx->height; y += 16) {
             for (int x = 0; x < avctx->width; x += 16) {
                 int idx;