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

[Michael Niedermayer]

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

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

What does censorship reveal? It reveals fear. -- Julian Assange
-------------- 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/20200516/22447eec/attachment.sig>