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

[Michael Niedermayer]

On Sat, May 16, 2020 at 07:12:28PM +0200, Paul B Mahol wrote:
> 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 ?

will apply an adjusted patch 

Thanks!

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- 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/6f47dc12/attachment.sig>