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