| Message ID | 20210927223722.25098-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avcodec/apedec: Fix undefined integer overflow in long_filter_ehigh_3830() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
On 9/27/2021 7:37 PM, Michael Niedermayer wrote: > Fixes: Timeout > Fixes: 39089/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSNSIREN_fuzzer-6677219854909440 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/siren.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavcodec/siren.c b/libavcodec/siren.c > index 7f2b4678608..708990d6654 100644 > --- a/libavcodec/siren.c > +++ b/libavcodec/siren.c > @@ -718,6 +718,9 @@ static int siren_decode(AVCodecContext *avctx, void *data, > if ((ret = init_get_bits8(gb, avpkt->data, avpkt->size)) < 0) > return ret; > > + if (s->sample_rate_bits + 4 > get_bits_left(gb)) > + return AVERROR_INVALIDDATA; This is not enough. You'll inevitably get another timeout in the future unless you add a get_bits_left(gb) > 0 condition to the loop in decode_envelope(). And there should be at least four bits left after decode_envelope() returns, so check for that too. > + > skip_bits(gb, s->sample_rate_bits); > > decode_envelope(s, gb, s->number_of_regions, >
On Tue, Sep 28, 2021 at 12:07:49AM -0300, James Almer wrote: > On 9/27/2021 7:37 PM, Michael Niedermayer wrote: > > Fixes: Timeout > > Fixes: 39089/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSNSIREN_fuzzer-6677219854909440 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/siren.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/libavcodec/siren.c b/libavcodec/siren.c > > index 7f2b4678608..708990d6654 100644 > > --- a/libavcodec/siren.c > > +++ b/libavcodec/siren.c > > @@ -718,6 +718,9 @@ static int siren_decode(AVCodecContext *avctx, void *data, > > if ((ret = init_get_bits8(gb, avpkt->data, avpkt->size)) < 0) > > return ret; > > + if (s->sample_rate_bits + 4 > get_bits_left(gb)) > > + return AVERROR_INVALIDDATA; > > This is not enough. You'll inevitably get another timeout in the future > unless you add a get_bits_left(gb) > 0 condition to the loop in > decode_envelope(). > And there should be at least four bits left after decode_envelope() returns, > so check for that too. suggest increasing the threshold to include: s->sample_rate_bits + s->number_of_regions + 4 + s->checksum_bits > get_bits_left(gb) hey michael are you only fuzzing AV_CODEC_ID_MSNSIREN? i would expect this issue to occur for for the vanilla SIREN codec too? -- Peter (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
On Tue, Sep 28, 2021 at 09:44:01PM +1000, Peter Ross wrote: > On Tue, Sep 28, 2021 at 12:07:49AM -0300, James Almer wrote: > > On 9/27/2021 7:37 PM, Michael Niedermayer wrote: > > > Fixes: Timeout > > > Fixes: 39089/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSNSIREN_fuzzer-6677219854909440 > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavcodec/siren.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/libavcodec/siren.c b/libavcodec/siren.c > > > index 7f2b4678608..708990d6654 100644 > > > --- a/libavcodec/siren.c > > > +++ b/libavcodec/siren.c > > > @@ -718,6 +718,9 @@ static int siren_decode(AVCodecContext *avctx, void *data, > > > if ((ret = init_get_bits8(gb, avpkt->data, avpkt->size)) < 0) > > > return ret; > > > + if (s->sample_rate_bits + 4 > get_bits_left(gb)) > > > + return AVERROR_INVALIDDATA; > > > > This is not enough. You'll inevitably get another timeout in the future > > unless you add a get_bits_left(gb) > 0 condition to the loop in > > decode_envelope(). > > And there should be at least four bits left after decode_envelope() returns, > > so check for that too. I tend not to add more checks than needed for timeouts because it increases the risk the patch gets rejected because it breaks something and then the reporter refuses to share any details. Or someone is offended because too many ugly/hacky timeout checks are added. So my patches are a bit minimalistic (less work and less friction). But that now offends others who want more complete fixes :( added a check in the loop, ill post the new patch > > suggest increasing the threshold to include: > > s->sample_rate_bits + s->number_of_regions + 4 + s->checksum_bits > get_bits_left(gb) changed > > > hey michael are you only fuzzing AV_CODEC_ID_MSNSIREN? > i would expect this issue to occur for for the vanilla SIREN codec too? The fuzzer is supposed to fuzz all decoders, sometimes it has difficulty with some decoders, when for example theres no seed file or something else special is needed for the codec. For example a codec may need a specific codec_tag or extradata set ... thx [...]
On 9/28/2021 3:03 PM, Michael Niedermayer wrote: > On Tue, Sep 28, 2021 at 09:44:01PM +1000, Peter Ross wrote: >> On Tue, Sep 28, 2021 at 12:07:49AM -0300, James Almer wrote: >>> On 9/27/2021 7:37 PM, Michael Niedermayer wrote: >>>> Fixes: Timeout >>>> Fixes: 39089/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSNSIREN_fuzzer-6677219854909440 >>>> >>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>>> --- >>>> libavcodec/siren.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/libavcodec/siren.c b/libavcodec/siren.c >>>> index 7f2b4678608..708990d6654 100644 >>>> --- a/libavcodec/siren.c >>>> +++ b/libavcodec/siren.c >>>> @@ -718,6 +718,9 @@ static int siren_decode(AVCodecContext *avctx, void *data, >>>> if ((ret = init_get_bits8(gb, avpkt->data, avpkt->size)) < 0) >>>> return ret; >>>> + if (s->sample_rate_bits + 4 > get_bits_left(gb)) >>>> + return AVERROR_INVALIDDATA; >>> >>> This is not enough. You'll inevitably get another timeout in the future >>> unless you add a get_bits_left(gb) > 0 condition to the loop in >>> decode_envelope(). >>> And there should be at least four bits left after decode_envelope() returns, >>> so check for that too. > > I tend not to add more checks than needed for timeouts because > it increases the risk the patch gets rejected because it breaks something and > then the reporter refuses to share any details. Or someone is offended because > too many ugly/hacky timeout checks are added. So my patches are a bit > minimalistic (less work and less friction). > But that now offends others who want more complete fixes :( > > added a check in the loop, ill post the new patch Nothing you did offended me. I'm just saying that your patch fixes the timeout for that specific bitstream created by the fuzzer, but the core issue is in that loop, and it'd be better to fix it there instead. If you don't want to add the check after decode_envelope() then that's ok. > > >> >> suggest increasing the threshold to include: >> > >> s->sample_rate_bits + s->number_of_regions + 4 + s->checksum_bits > get_bits_left(gb) > > changed > > >> >> >> hey michael are you only fuzzing AV_CODEC_ID_MSNSIREN? >> i would expect this issue to occur for for the vanilla SIREN codec too? > > The fuzzer is supposed to fuzz all decoders, > sometimes it has difficulty with some decoders, when for example theres no > seed file or something else special is needed for the codec. For example > a codec may need a specific codec_tag or extradata set ... > > thx > > [...] > > > _______________________________________________ > 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". >
On Tue, Sep 28, 2021 at 8:09 PM James Almer <jamrial@gmail.com> wrote: > On 9/28/2021 3:03 PM, Michael Niedermayer wrote: > > On Tue, Sep 28, 2021 at 09:44:01PM +1000, Peter Ross wrote: > >> On Tue, Sep 28, 2021 at 12:07:49AM -0300, James Almer wrote: > >>> On 9/27/2021 7:37 PM, Michael Niedermayer wrote: > >>>> Fixes: Timeout > >>>> Fixes: > 39089/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSNSIREN_fuzzer-6677219854909440 > >>>> > >>>> Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >>>> --- > >>>> libavcodec/siren.c | 3 +++ > >>>> 1 file changed, 3 insertions(+) > >>>> > >>>> diff --git a/libavcodec/siren.c b/libavcodec/siren.c > >>>> index 7f2b4678608..708990d6654 100644 > >>>> --- a/libavcodec/siren.c > >>>> +++ b/libavcodec/siren.c > >>>> @@ -718,6 +718,9 @@ static int siren_decode(AVCodecContext *avctx, > void *data, > >>>> if ((ret = init_get_bits8(gb, avpkt->data, avpkt->size)) < 0) > >>>> return ret; > >>>> + if (s->sample_rate_bits + 4 > get_bits_left(gb)) > >>>> + return AVERROR_INVALIDDATA; > >>> > >>> This is not enough. You'll inevitably get another timeout in the future > >>> unless you add a get_bits_left(gb) > 0 condition to the loop in > >>> decode_envelope(). > >>> And there should be at least four bits left after decode_envelope() > returns, > >>> so check for that too. > > > > I tend not to add more checks than needed for timeouts because > > it increases the risk the patch gets rejected because it breaks > something and > > then the reporter refuses to share any details. Or someone is offended > because > > too many ugly/hacky timeout checks are added. So my patches are a bit > > minimalistic (less work and less friction). > > But that now offends others who want more complete fixes :( > > > > added a check in the loop, ill post the new patch > > Nothing you did offended me. I'm just saying that your patch fixes the > timeout for that specific bitstream created by the fuzzer, but the core > issue is in that loop, and it'd be better to fix it there instead. > > If you don't want to add the check after decode_envelope() then that's ok. > I prefer James solution, instead of heuristic nonsense. > > > > > > >> > >> suggest increasing the threshold to include: > >> > > > >> s->sample_rate_bits + s->number_of_regions + 4 + s->checksum_bits > > get_bits_left(gb) > > > > changed > > > > > >> > >> > >> hey michael are you only fuzzing AV_CODEC_ID_MSNSIREN? > >> i would expect this issue to occur for for the vanilla SIREN codec too? > > > > The fuzzer is supposed to fuzz all decoders, > > sometimes it has difficulty with some decoders, when for example theres > no > > seed file or something else special is needed for the codec. For example > > a codec may need a specific codec_tag or extradata set ... > > > > thx > > > > [...] > > > > > > _______________________________________________ > > 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". > > > > _______________________________________________ > 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". >
diff --git a/libavcodec/siren.c b/libavcodec/siren.c index 7f2b4678608..708990d6654 100644 --- a/libavcodec/siren.c +++ b/libavcodec/siren.c @@ -718,6 +718,9 @@ static int siren_decode(AVCodecContext *avctx, void *data, if ((ret = init_get_bits8(gb, avpkt->data, avpkt->size)) < 0) return ret; + if (s->sample_rate_bits + 4 > get_bits_left(gb)) + return AVERROR_INVALIDDATA; + skip_bits(gb, s->sample_rate_bits); decode_envelope(s, gb, s->number_of_regions,
Fixes: Timeout Fixes: 39089/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSNSIREN_fuzzer-6677219854909440 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/siren.c | 3 +++ 1 file changed, 3 insertions(+)