diff mbox

[FFmpeg-devel,2/3] avcodec/iff: Check bpp for validity

Message ID 20190618235303.8738-2-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer June 18, 2019, 11:53 p.m. UTC 
Fixes: shift exponent -100663046 is negative
Fixes: out of array access
Fixes: 15270/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_IFF_ILBM_fuzzer-5727829913763840

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

Comments

Peter Ross June 19, 2019, 9:34 a.m. UTC | #1 
On Wed, Jun 19, 2019 at 01:53:02AM +0200, Michael Niedermayer wrote:
> Fixes: shift exponent -100663046 is negative
> Fixes: out of array access
> Fixes: 15270/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_IFF_ILBM_fuzzer-5727829913763840
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/iff.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/iff.c b/libavcodec/iff.c
> index 33cf2e3a94..48a0340604 100644
> --- a/libavcodec/iff.c
> +++ b/libavcodec/iff.c
> @@ -382,7 +382,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
>      IffContext *s = avctx->priv_data;
>      int err;
>  
> -    if (avctx->bits_per_coded_sample <= 8) {
> +    if (avctx->bits_per_coded_sample <= 8U) {
>          int palette_size;
>  
>          if (avctx->extradata_size >= 2)
> @@ -391,7 +391,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
>              palette_size = 0;
>          avctx->pix_fmt = (avctx->bits_per_coded_sample < 8) ||
>                           (avctx->extradata_size >= 2 && palette_size) ? AV_PIX_FMT_PAL8 : AV_PIX_FMT_GRAY8;
> -    } else if (avctx->bits_per_coded_sample <= 32) {
> +    } else if (avctx->bits_per_coded_sample <= 32U) {
>          if (avctx->codec_tag == MKTAG('R', 'G', 'B', '8')) {
>              avctx->pix_fmt = AV_PIX_FMT_RGB32;
>          } else if (avctx->codec_tag == MKTAG('R', 'G', 'B', 'N')) {
> -- 
> 2.21.0

hey michael, can you post the test case file?

AVCodecParameters.bits_per_coded_sample is signed, but looking at *libavformat*/iff.c,
i can't fathom how its getting below zero.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
Michael Niedermayer June 19, 2019, 9:44 a.m. UTC | #2 
On Wed, Jun 19, 2019 at 07:34:19PM +1000, Peter Ross wrote:
> On Wed, Jun 19, 2019 at 01:53:02AM +0200, Michael Niedermayer wrote:
> > Fixes: shift exponent -100663046 is negative
> > Fixes: out of array access
> > Fixes: 15270/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_IFF_ILBM_fuzzer-5727829913763840
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/iff.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavcodec/iff.c b/libavcodec/iff.c
> > index 33cf2e3a94..48a0340604 100644
> > --- a/libavcodec/iff.c
> > +++ b/libavcodec/iff.c
> > @@ -382,7 +382,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
> >      IffContext *s = avctx->priv_data;
> >      int err;
> >  
> > -    if (avctx->bits_per_coded_sample <= 8) {
> > +    if (avctx->bits_per_coded_sample <= 8U) {
> >          int palette_size;
> >  
> >          if (avctx->extradata_size >= 2)
> > @@ -391,7 +391,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
> >              palette_size = 0;
> >          avctx->pix_fmt = (avctx->bits_per_coded_sample < 8) ||
> >                           (avctx->extradata_size >= 2 && palette_size) ? AV_PIX_FMT_PAL8 : AV_PIX_FMT_GRAY8;
> > -    } else if (avctx->bits_per_coded_sample <= 32) {
> > +    } else if (avctx->bits_per_coded_sample <= 32U) {
> >          if (avctx->codec_tag == MKTAG('R', 'G', 'B', '8')) {
> >              avctx->pix_fmt = AV_PIX_FMT_RGB32;
> >          } else if (avctx->codec_tag == MKTAG('R', 'G', 'B', 'N')) {
> > -- 
> > 2.21.0
> 
> hey michael, can you post the test case file?
>       
> AVCodecParameters.bits_per_coded_sample is signed, but looking at *libavformat*/iff.c,
> i can't fathom how its getting below zero.

even better i can explain it
in target_dec_fuzzer.c there is:
        ctx->bits_per_coded_sample              = bytestream2_get_le32(&gbc);

quite possibly this is not possble with libavformat and this codec currently.
A API user from his own demuxer or a future demuxer in libavformat could set
it though.

Thanks

[...]
Peter Ross June 19, 2019, 10:11 a.m. UTC | #3 
On Wed, Jun 19, 2019 at 11:44:36AM +0200, Michael Niedermayer wrote:
> On Wed, Jun 19, 2019 at 07:34:19PM +1000, Peter Ross wrote:
> > On Wed, Jun 19, 2019 at 01:53:02AM +0200, Michael Niedermayer wrote:
> > > Fixes: shift exponent -100663046 is negative
> > > Fixes: out of array access
> > > Fixes: 15270/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_IFF_ILBM_fuzzer-5727829913763840
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavcodec/iff.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/libavcodec/iff.c b/libavcodec/iff.c
> > > index 33cf2e3a94..48a0340604 100644
> > > --- a/libavcodec/iff.c
> > > +++ b/libavcodec/iff.c
> > > @@ -382,7 +382,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
> > >      IffContext *s = avctx->priv_data;
> > >      int err;
> > >  
> > > -    if (avctx->bits_per_coded_sample <= 8) {
> > > +    if (avctx->bits_per_coded_sample <= 8U) {
> > >          int palette_size;
> > >  
> > >          if (avctx->extradata_size >= 2)
> > > @@ -391,7 +391,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
> > >              palette_size = 0;
> > >          avctx->pix_fmt = (avctx->bits_per_coded_sample < 8) ||
> > >                           (avctx->extradata_size >= 2 && palette_size) ? AV_PIX_FMT_PAL8 : AV_PIX_FMT_GRAY8;
> > > -    } else if (avctx->bits_per_coded_sample <= 32) {
> > > +    } else if (avctx->bits_per_coded_sample <= 32U) {
> > >          if (avctx->codec_tag == MKTAG('R', 'G', 'B', '8')) {
> > >              avctx->pix_fmt = AV_PIX_FMT_RGB32;
> > >          } else if (avctx->codec_tag == MKTAG('R', 'G', 'B', 'N')) {
> > > -- 
> > > 2.21.0
> > 
> > hey michael, can you post the test case file?
> >       
> > AVCodecParameters.bits_per_coded_sample is signed, but looking at *libavformat*/iff.c,
> > i can't fathom how its getting below zero.
> 
> even better i can explain it
> in target_dec_fuzzer.c there is:
>         ctx->bits_per_coded_sample              = bytestream2_get_le32(&gbc);
> 
> quite possibly this is not possble with libavformat and this codec currently.
> A API user from his own demuxer or a future demuxer in libavformat could set
> it though.

understood, this patch is ok.

i fear there are many of other places where signed test is done on bits_per_coded sample
leading to shift explosion. given enough time, fuzz will eventually catch them all.

how radical of a change would just making AVCodecParameters.bits_per_coded_sample unsigned be?

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
Michael Niedermayer June 19, 2019, 6:45 p.m. UTC | #4 
On Wed, Jun 19, 2019 at 08:11:50PM +1000, Peter Ross wrote:
> On Wed, Jun 19, 2019 at 11:44:36AM +0200, Michael Niedermayer wrote:
> > On Wed, Jun 19, 2019 at 07:34:19PM +1000, Peter Ross wrote:
> > > On Wed, Jun 19, 2019 at 01:53:02AM +0200, Michael Niedermayer wrote:
> > > > Fixes: shift exponent -100663046 is negative
> > > > Fixes: out of array access
> > > > Fixes: 15270/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_IFF_ILBM_fuzzer-5727829913763840
> > > > 
> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  libavcodec/iff.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/libavcodec/iff.c b/libavcodec/iff.c
> > > > index 33cf2e3a94..48a0340604 100644
> > > > --- a/libavcodec/iff.c
> > > > +++ b/libavcodec/iff.c
> > > > @@ -382,7 +382,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
> > > >      IffContext *s = avctx->priv_data;
> > > >      int err;
> > > >  
> > > > -    if (avctx->bits_per_coded_sample <= 8) {
> > > > +    if (avctx->bits_per_coded_sample <= 8U) {
> > > >          int palette_size;
> > > >  
> > > >          if (avctx->extradata_size >= 2)
> > > > @@ -391,7 +391,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
> > > >              palette_size = 0;
> > > >          avctx->pix_fmt = (avctx->bits_per_coded_sample < 8) ||
> > > >                           (avctx->extradata_size >= 2 && palette_size) ? AV_PIX_FMT_PAL8 : AV_PIX_FMT_GRAY8;
> > > > -    } else if (avctx->bits_per_coded_sample <= 32) {
> > > > +    } else if (avctx->bits_per_coded_sample <= 32U) {
> > > >          if (avctx->codec_tag == MKTAG('R', 'G', 'B', '8')) {
> > > >              avctx->pix_fmt = AV_PIX_FMT_RGB32;
> > > >          } else if (avctx->codec_tag == MKTAG('R', 'G', 'B', 'N')) {
> > > > -- 
> > > > 2.21.0
> > > 
> > > hey michael, can you post the test case file?
> > >       
> > > AVCodecParameters.bits_per_coded_sample is signed, but looking at *libavformat*/iff.c,
> > > i can't fathom how its getting below zero.
> > 
> > even better i can explain it
> > in target_dec_fuzzer.c there is:
> >         ctx->bits_per_coded_sample              = bytestream2_get_le32(&gbc);
> > 
> > quite possibly this is not possble with libavformat and this codec currently.
> > A API user from his own demuxer or a future demuxer in libavformat could set
> > it though.
> 
> understood, this patch is ok.
> 
> i fear there are many of other places where signed test is done on bits_per_coded sample
> leading to shift explosion. given enough time, fuzz will eventually catch them all.
> 
> how radical of a change would just making AVCodecParameters.bits_per_coded_sample unsigned be?

Ive just posted a patch that checks this for all decoders in a central place
this should avoid each decoder having to deal with it

Thanks

[...]
diff mbox

Patch

diff --git a/libavcodec/iff.c b/libavcodec/iff.c
index 33cf2e3a94..48a0340604 100644
--- a/libavcodec/iff.c
+++ b/libavcodec/iff.c
@@ -382,7 +382,7 @@  static av_cold int decode_init(AVCodecContext *avctx)
     IffContext *s = avctx->priv_data;
     int err;
 
-    if (avctx->bits_per_coded_sample <= 8) {
+    if (avctx->bits_per_coded_sample <= 8U) {
         int palette_size;
 
         if (avctx->extradata_size >= 2)
@@ -391,7 +391,7 @@  static av_cold int decode_init(AVCodecContext *avctx)
             palette_size = 0;
         avctx->pix_fmt = (avctx->bits_per_coded_sample < 8) ||
                          (avctx->extradata_size >= 2 && palette_size) ? AV_PIX_FMT_PAL8 : AV_PIX_FMT_GRAY8;
-    } else if (avctx->bits_per_coded_sample <= 32) {
+    } else if (avctx->bits_per_coded_sample <= 32U) {
         if (avctx->codec_tag == MKTAG('R', 'G', 'B', '8')) {
             avctx->pix_fmt = AV_PIX_FMT_RGB32;
         } else if (avctx->codec_tag == MKTAG('R', 'G', 'B', 'N')) {