| Message ID | 20190622223055.19020-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
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)
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 [...]
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 [...]
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);
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(-)