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

[Michael Niedermayer]

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 ...

Thanks

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200420/a23c6a10/attachment.sig>