diff mbox series

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

Message ID 20200114143645.10358-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel] avcodec/binkaudio: Check sample_rate to avoid integer overflow
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Michael Niedermayer Jan. 14, 2020, 2:36 p.m. UTC
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@niedermayer.cc>
---
 libavcodec/binkaudio.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Paul B Mahol Jan. 14, 2020, 3:04 p.m. UTC | #1
This better belong to generic code.

On 1/14/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> 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@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;
> --
> 2.24.0
>
> _______________________________________________
> 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".
Michael Niedermayer Feb. 1, 2020, 3:14 p.m. UTC | #2
On Tue, Jan 14, 2020 at 04:04:29PM +0100, Paul B Mahol wrote:
> This better belong to generic code.

This specific check (which checks for INT_MAX) is specific to our
bink audio code which does a +1
so it would not fit in generic code

We could arbitrarily decide on a maximum sample rate hardcode that 
and check for that in generic code.
I can implement that if people prefer. It would not avoid all
sample rate checks in codecs though ...

Thanks

[...]
Paul B Mahol Feb. 1, 2020, 3:17 p.m. UTC | #3
On 2/1/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Tue, Jan 14, 2020 at 04:04:29PM +0100, Paul B Mahol wrote:
>> This better belong to generic code.
>
> This specific check (which checks for INT_MAX) is specific to our
> bink audio code which does a +1
> so it would not fit in generic code
>
> We could arbitrarily decide on a maximum sample rate hardcode that
> and check for that in generic code.
> I can implement that if people prefer. It would not avoid all
> sample rate checks in codecs though ...

sample rate can not be > INT_MAX

> Thanks
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> In a rich man's house there is no place to spit but his face.
> -- Diogenes of Sinope
>
Michael Niedermayer Feb. 1, 2020, 10:48 p.m. UTC | #4
On Sat, Feb 01, 2020 at 04:17:10PM +0100, Paul B Mahol wrote:
> On 2/1/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Tue, Jan 14, 2020 at 04:04:29PM +0100, Paul B Mahol wrote:
> >> This better belong to generic code.
> >
> > This specific check (which checks for INT_MAX) is specific to our
> > bink audio code which does a +1
> > so it would not fit in generic code
> >
> > We could arbitrarily decide on a maximum sample rate hardcode that
> > and check for that in generic code.
> > I can implement that if people prefer. It would not avoid all
> > sample rate checks in codecs though ...
> 
> sample rate can not be > INT_MAX

no and the code also doesnt check > INT_MAX 
I think you maybe missed the = in >=
theres a +1 and INT_MAX+1 is bad so INT_MAX is checked for
we can do that in generic code but its only this decoder that has this
issue other decoders may have other limits. That makes this specific
check threshold bad for a check in generic code. Another threshold
would work in generic code, it would be arbitrary though and limit
most decoders more than needed
Iam happy to implement what people prefer but the check as it is
makes not much sense if its moved as is into generic code

Thanks

[...]
Michael Niedermayer Feb. 9, 2020, 8:28 p.m. UTC | #5
On Sat, Feb 01, 2020 at 11:48:06PM +0100, Michael Niedermayer wrote:
> On Sat, Feb 01, 2020 at 04:17:10PM +0100, Paul B Mahol wrote:
> > On 2/1/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > > On Tue, Jan 14, 2020 at 04:04:29PM +0100, Paul B Mahol wrote:
> > >> This better belong to generic code.
> > >
> > > This specific check (which checks for INT_MAX) is specific to our
> > > bink audio code which does a +1
> > > so it would not fit in generic code
> > >
> > > We could arbitrarily decide on a maximum sample rate hardcode that
> > > and check for that in generic code.
> > > I can implement that if people prefer. It would not avoid all
> > > sample rate checks in codecs though ...
> > 
> > sample rate can not be > INT_MAX
> 
> no and the code also doesnt check > INT_MAX 
> I think you maybe missed the = in >=
> theres a +1 and INT_MAX+1 is bad so INT_MAX is checked for
> we can do that in generic code but its only this decoder that has this
> issue other decoders may have other limits. That makes this specific
> check threshold bad for a check in generic code. Another threshold
> would work in generic code, it would be arbitrary though and limit
> most decoders more than needed
> Iam happy to implement what people prefer but the check as it is
> makes not much sense if its moved as is into generic code

any preferrance on how to solve this ?
or you are ok with the patch ?

thanks

[...]
Michael Niedermayer April 4, 2020, 9:38 p.m. UTC | #6
On Sun, Feb 09, 2020 at 09:28:48PM +0100, Michael Niedermayer wrote:
> On Sat, Feb 01, 2020 at 11:48:06PM +0100, Michael Niedermayer wrote:
> > On Sat, Feb 01, 2020 at 04:17:10PM +0100, Paul B Mahol wrote:
> > > On 2/1/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > > > On Tue, Jan 14, 2020 at 04:04:29PM +0100, Paul B Mahol wrote:
> > > >> This better belong to generic code.
> > > >
> > > > This specific check (which checks for INT_MAX) is specific to our
> > > > bink audio code which does a +1
> > > > so it would not fit in generic code
> > > >
> > > > We could arbitrarily decide on a maximum sample rate hardcode that
> > > > and check for that in generic code.
> > > > I can implement that if people prefer. It would not avoid all
> > > > sample rate checks in codecs though ...
> > > 
> > > sample rate can not be > INT_MAX
> > 
> > no and the code also doesnt check > INT_MAX 
> > I think you maybe missed the = in >=
> > theres a +1 and INT_MAX+1 is bad so INT_MAX is checked for
> > we can do that in generic code but its only this decoder that has this
> > issue other decoders may have other limits. That makes this specific
> > check threshold bad for a check in generic code. Another threshold
> > would work in generic code, it would be arbitrary though and limit
> > most decoders more than needed
> > Iam happy to implement what people prefer but the check as it is
> > makes not much sense if its moved as is into generic code
> 
> any preferrance on how to solve this ?
> or you are ok with the patch ?

ping

[...]
Michael Niedermayer April 7, 2020, 9:17 p.m. UTC | #7
On Sat, Apr 04, 2020 at 11:38:43PM +0200, Michael Niedermayer wrote:
> On Sun, Feb 09, 2020 at 09:28:48PM +0100, Michael Niedermayer wrote:
> > On Sat, Feb 01, 2020 at 11:48:06PM +0100, Michael Niedermayer wrote:
> > > On Sat, Feb 01, 2020 at 04:17:10PM +0100, Paul B Mahol wrote:
> > > > On 2/1/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > > > > On Tue, Jan 14, 2020 at 04:04:29PM +0100, Paul B Mahol wrote:
> > > > >> This better belong to generic code.
> > > > >
> > > > > This specific check (which checks for INT_MAX) is specific to our
> > > > > bink audio code which does a +1
> > > > > so it would not fit in generic code
> > > > >
> > > > > We could arbitrarily decide on a maximum sample rate hardcode that
> > > > > and check for that in generic code.
> > > > > I can implement that if people prefer. It would not avoid all
> > > > > sample rate checks in codecs though ...
> > > > 
> > > > sample rate can not be > INT_MAX
> > > 
> > > no and the code also doesnt check > INT_MAX 
> > > I think you maybe missed the = in >=
> > > theres a +1 and INT_MAX+1 is bad so INT_MAX is checked for
> > > we can do that in generic code but its only this decoder that has this
> > > issue other decoders may have other limits. That makes this specific
> > > check threshold bad for a check in generic code. Another threshold
> > > would work in generic code, it would be arbitrary though and limit
> > > most decoders more than needed
> > > Iam happy to implement what people prefer but the check as it is
> > > makes not much sense if its moved as is into generic code
> > 
> > any preferrance on how to solve this ?
> > or you are ok with the patch ?
> 
> ping

paul, are you ok with me applying the patch ? it seems to me that its the
most obvious fix here

thanks

[...]
diff mbox series

Patch

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;