[FFmpeg-devel] [PATCH 2/2] avcodec/binkaudio: Check sample_rate to avoid integer overflow

[Paul B Mahol]

On 5/16/20, Michael Niedermayer <michael at niedermayer.cc> wrote:
> On Mon, Apr 20, 2020 at 01:03:34AM +0200, Michael Niedermayer wrote:
>> On Sun, Apr 19, 2020 at 05:52:01PM +0200, Lynne wrote:
>> > Apr 19, 2020, 16:05 by michael at niedermayer.cc:
>> >
>> > > Fixes: signed integer overflow: 2147483647 + 1 cannot be represented
>> > > in type 'int'
>> > > Fixes:
>> > > 19950/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_BINKAUDIO_DCT_fuzzer-5765514337189888
>> > >
>> > > Found-by: continuous fuzzing process
>> > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>> > > ---
>> > >  libavcodec/binkaudio.c | 3 +++
>> > >  1 file changed, 3 insertions(+)
>> > >
>> > > diff --git a/libavcodec/binkaudio.c b/libavcodec/binkaudio.c
>> > > index 64a08b8608..2df3dc645a 100644
>> > > --- a/libavcodec/binkaudio.c
>> > > +++ b/libavcodec/binkaudio.c
>> > > @@ -106,6 +106,9 @@ static av_cold int decode_init(AVCodecContext
>> > > *avctx)
>> > >  avctx->sample_fmt = AV_SAMPLE_FMT_FLTP;
>> > >  }
>> > >
>> > > +    if (sample_rate >= INT_MAX)
>> > > +        return AVERROR_INVALIDDATA;
>> > > +
>> > >  s->frame_len     = 1 << frame_len_bits;
>> > >  s->overlap_len   = s->frame_len / 16;
>> > >  s->block_size    = (s->frame_len - s->overlap_len) * s->channels;
>> > >
>> >
>> > Did you even bother to look at the checks you added in this decoder
>> > previously?
>> > Specifically 11 lines above?
>> >
>> > > if (sample_rate > INT_MAX / avctx->channels)
>> > >     return AVERROR_INVALIDDATA;
>> > > sample_rate  *= avctx->channels;
>> >
>> > To start with the sample rate of the avctx is already checked in
>> > utils.c, and you
>> > still haven't cleaned up any decoders from the checks made unnecessary
>> > by you,
>>
>> The new check is needed, it overflows without:
>> libavcodec/binkaudio.c:116:37: runtime error: signed integer overflow:
>> 2147483647 + 1 cannot be represented in type 'int'
>>
>> The other check is also needed, if i remove it it still overflows
>> libavcodec/binkaudio.c:100:22: runtime error: signed integer overflow:
>> 1092624416 * 2 cannot be represented in type 'int'
>>
>>
>> > so am reminding you again to clean up the codebase by getting rid of
>> > them.
>> > At least you might get to clean the codebase for once rather than adding
>> > crap like this.
>> >
>> > So there's only the branch which I quoted that's needed to be fixed, and
>> > since there's a
>> > check there already, there's no reason to have a check here as well.
>>
>> I posted exactly this same patch in january, there was a objection, and i
>> asked
>> what was preferred instead, and pinged it multiple times over the
>> following
>> months, in february and april:
>>
>> 189192 N F 0114 15:36 To FFmpeg devel (1,6K) [FFmpeg-devel] [PATCH]
>> avcodec/binkaudio: Check sample_rate to avoid integer overflow
>> 189193 r   0114 16:04 Paul B Mahol    (2,1K) └─>
>> 189194  sF 0201 16:14 To FFmpeg devel (1,7K)   └─>
>> 189195 r   0201 16:17 Paul B Mahol    (1,3K)     └─>
>> 189196 rsF 0201 23:48 To FFmpeg devel (2,6K)       └─>
>> 189197 rs  0209 21:28 Michael Niederm (2,9K)         └─>
>> 189198 rsF 0404 23:38 To FFmpeg devel (2,8K)           └─>
>> 189199  sF 0407 23:17 To FFmpeg devel (3,1K)             └─>
>>
>> but i failed to get a reply, so i tried reposting it
>>
>> So, if the patch is still not liked, can you explain what do you
>> prefer ?
>>
>> i can put the sample_rate >= INT_MAX check in generic code but it
>> is specific to this decoder it wont help anything else
>>
>> i can put a more aggressive check like sample_rate * channels >
>> MAX_CHANNEL_SAMPLES
>> in generic code, that will of course block some otherwise supported cases
>>
>> or we can have checks just for what is not supported
>>
>> or we can extend the code to support a wider range where it is possible
>>
>
>> Just say what you prefer, i dont mind at all what it is, i just want the
>> issue fixed its already so many months open ...
>
> ping
> id like to fix ossfuzz issue 19950

Use (sample_rate + 1LL) / 2 ?

>
> thx
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> What does censorship reveal? It reveals fear. -- Julian Assange
>