[FFmpeg-devel,2/3] avcodec/iff: Check ham vs bpp

Submitted by Michael Niedermayer on June 22, 2019, 10:30 p.m.

Details

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

Commit Message

Michael Niedermayer June 22, 2019, 10:30 p.m.
This checks the ham value much stricter and avoids hitting cases which cannot be reached
with data from the libavformat demuxer.

Fixes: out of array access
Fixes: 15320/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_IFF_ILBM_fuzzer-5080476840099840
Fixes: 15423/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_IFF_ILBM_fuzzer-5630765833912320

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 | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Peter Ross June 23, 2019, 8:33 a.m.
On Sun, Jun 23, 2019 at 12:30:54AM +0200, Michael Niedermayer wrote:
> This checks the ham value much stricter and avoids hitting cases which cannot be reached
> with data from the libavformat demuxer.
> 
> Fixes: out of array access
> Fixes: 15320/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_IFF_ILBM_fuzzer-5080476840099840
> Fixes: 15423/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_IFF_ILBM_fuzzer-5630765833912320
> 
> 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 | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/iff.c b/libavcodec/iff.c
> index b43bd507b3..b065bbe598 100644
> --- a/libavcodec/iff.c
> +++ b/libavcodec/iff.c
> @@ -280,6 +280,18 @@ static int extract_header(AVCodecContext *const avctx,
>          for (i = 0; i < 16; i++)
>              s->tvdc[i] = bytestream_get_be16(&buf);
>  
> +        if (s->ham) {
> +            if (s->bpp > 8) {
> +                av_log(avctx, AV_LOG_ERROR, "Invalid number of hold bits for HAM: %u\n", s->ham);
> +                s->ham = 0;
> +                return AVERROR_INVALIDDATA;
> +            } if (s->ham != (s->bpp > 6 ? 6 : 4)) {
> +                av_log(avctx, AV_LOG_ERROR, "Invalid number of hold bits for HAM: %u, BPP: %u\n", s->ham, s->bpp);
> +                s->ham = 0;
> +                return AVERROR_INVALIDDATA;
> +            }
> +        }

i am curious why we need to reset ham = 0? otherwise patch okay.

patch 1 and 3 okay.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
Michael Niedermayer June 23, 2019, 8:45 a.m.
On Sun, Jun 23, 2019 at 06:33:02PM +1000, Peter Ross wrote:
> On Sun, Jun 23, 2019 at 12:30:54AM +0200, Michael Niedermayer wrote:
> > This checks the ham value much stricter and avoids hitting cases which cannot be reached
> > with data from the libavformat demuxer.
> > 
> > Fixes: out of array access
> > Fixes: 15320/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_IFF_ILBM_fuzzer-5080476840099840
> > Fixes: 15423/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_IFF_ILBM_fuzzer-5630765833912320
> > 
> > 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 | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavcodec/iff.c b/libavcodec/iff.c
> > index b43bd507b3..b065bbe598 100644
> > --- a/libavcodec/iff.c
> > +++ b/libavcodec/iff.c
> > @@ -280,6 +280,18 @@ static int extract_header(AVCodecContext *const avctx,
> >          for (i = 0; i < 16; i++)
> >              s->tvdc[i] = bytestream_get_be16(&buf);
> >  
> > +        if (s->ham) {
> > +            if (s->bpp > 8) {
> > +                av_log(avctx, AV_LOG_ERROR, "Invalid number of hold bits for HAM: %u\n", s->ham);
> > +                s->ham = 0;
> > +                return AVERROR_INVALIDDATA;
> > +            } if (s->ham != (s->bpp > 6 ? 6 : 4)) {
> > +                av_log(avctx, AV_LOG_ERROR, "Invalid number of hold bits for HAM: %u, BPP: %u\n", s->ham, s->bpp);
> > +                s->ham = 0;
> > +                return AVERROR_INVALIDDATA;
> > +            }
> > +        }
> 
> i am curious why we need to reset ham = 0? otherwise patch okay.

That was just so as not to leave an invalid value in the context. It is
very likely not needed, I will remove it.


> 
> patch 1 and 3 okay.

will apply

thx


[...]
Michael Niedermayer July 8, 2019, 7:36 a.m.
On Sun, Jun 23, 2019 at 10:45:56AM +0200, Michael Niedermayer wrote:
> On Sun, Jun 23, 2019 at 06:33:02PM +1000, Peter Ross wrote:
> > On Sun, Jun 23, 2019 at 12:30:54AM +0200, Michael Niedermayer wrote:
> > > This checks the ham value much stricter and avoids hitting cases which cannot be reached
> > > with data from the libavformat demuxer.
> > > 
> > > Fixes: out of array access
> > > Fixes: 15320/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_IFF_ILBM_fuzzer-5080476840099840
> > > Fixes: 15423/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_IFF_ILBM_fuzzer-5630765833912320
> > > 
> > > 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 | 15 ++++++++++++---
> > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/libavcodec/iff.c b/libavcodec/iff.c
> > > index b43bd507b3..b065bbe598 100644
> > > --- a/libavcodec/iff.c
> > > +++ b/libavcodec/iff.c
> > > @@ -280,6 +280,18 @@ static int extract_header(AVCodecContext *const avctx,
> > >          for (i = 0; i < 16; i++)
> > >              s->tvdc[i] = bytestream_get_be16(&buf);
> > >  
> > > +        if (s->ham) {
> > > +            if (s->bpp > 8) {
> > > +                av_log(avctx, AV_LOG_ERROR, "Invalid number of hold bits for HAM: %u\n", s->ham);
> > > +                s->ham = 0;
> > > +                return AVERROR_INVALIDDATA;
> > > +            } if (s->ham != (s->bpp > 6 ? 6 : 4)) {
> > > +                av_log(avctx, AV_LOG_ERROR, "Invalid number of hold bits for HAM: %u, BPP: %u\n", s->ham, s->bpp);
> > > +                s->ham = 0;
> > > +                return AVERROR_INVALIDDATA;
> > > +            }
> > > +        }
> > 
> > i am curious why we need to reset ham = 0? otherwise patch okay.
> 
> That was just so as not to leave an invalid value in the context. It is
> very likely not needed, I will remove it.

will apply without the ham = 0


[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/iff.c b/libavcodec/iff.c
index b43bd507b3..b065bbe598 100644
--- a/libavcodec/iff.c
+++ b/libavcodec/iff.c
@@ -280,6 +280,18 @@  static int extract_header(AVCodecContext *const avctx,
         for (i = 0; i < 16; i++)
             s->tvdc[i] = bytestream_get_be16(&buf);
 
+        if (s->ham) {
+            if (s->bpp > 8) {
+                av_log(avctx, AV_LOG_ERROR, "Invalid number of hold bits for HAM: %u\n", s->ham);
+                s->ham = 0;
+                return AVERROR_INVALIDDATA;
+            } if (s->ham != (s->bpp > 6 ? 6 : 4)) {
+                av_log(avctx, AV_LOG_ERROR, "Invalid number of hold bits for HAM: %u, BPP: %u\n", s->ham, s->bpp);
+                s->ham = 0;
+                return AVERROR_INVALIDDATA;
+            }
+        }
+
         if (s->masking == MASK_HAS_MASK) {
             if (s->bpp >= 8 && !s->ham) {
                 avctx->pix_fmt = AV_PIX_FMT_RGB32;
@@ -307,9 +319,6 @@  static int extract_header(AVCodecContext *const avctx,
         if (!s->bpp || s->bpp > 32) {
             av_log(avctx, AV_LOG_ERROR, "Invalid number of bitplanes: %u\n", s->bpp);
             return AVERROR_INVALIDDATA;
-        } else if (s->ham >= 8) {
-            av_log(avctx, AV_LOG_ERROR, "Invalid number of hold bits for HAM: %u\n", s->ham);
-            return AVERROR_INVALIDDATA;
         }
 
         av_freep(&s->ham_buf);