Re: [FFmpeg-devel] [PATCH] avcodec/bonk: Check step

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 2614 bytes --]

On Sun, Oct 02, 2022 at 07:13:39PM +0300, Rémi Denis-Courmont wrote:
> Le sunnuntaina 2. lokakuuta 2022, 18.43.23 EEST Michael Niedermayer a écrit :
> > Fixes: signed integer overflow: 2040812214 + 255101526 cannot be represented
> > in type 'int' Fixes:
> > 51323/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_BONK_fuzzer-4791481
> > 067503616
> > 
> > Found-by: continuous fuzzing process
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/bonk.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/libavcodec/bonk.c b/libavcodec/bonk.c
> > index 409694f710d..32f7c9b9bdb 100644
> > --- a/libavcodec/bonk.c
> > +++ b/libavcodec/bonk.c
> > @@ -187,6 +187,9 @@ static int intlist_read(BonkContext *s, int *buf, int
> > entries, int base_2_part) if (!dominant)
> >                  n_zeros += steplet;
> > 
> > +            if (step > INT_MAX/9*8)

if you want the exact limit for any INT_MAX its:
(INT_MAX + 1ULL) / 9 * 8 + (INT_MAX + 1ULL) % 9 - 1;

though probably a fixed bit value would be better anyway for reproducability


> > +                return AVERROR_INVALIDDATA;
> > +
> >              step += step / 8;
> >          } else if (steplet > 0) {
> >              int actual_run = read_uint_max(s, steplet - 1);
> 
> No problem with this patch *specifically* but wouldn't it be more effective to 
> fix that sort of issue with checked arithmetic, e.g. something like:
> 
>         if (av_ckd_add(&step, step, step / 8))
>             return AVERROR_INVALIDDATA;
> 
> ...especially on 64-bit systems whence this is really just an add. This also 
> avoids having to figure out what the exact boundary value is.

If someone has a set of testfiles for this decoder then I can look at this
and cleanup the code more completely.
i dont know if a step > C vs overflow check is better but i have the suspicion 
there are more problems as multiple statments around this seem unreachable.
And iam also missing some check that i would expect to be in there 

I found links to datafilehost.com to test files but they are all expired
so i have no test files, so iam a bit cautious with changes ...

CCing piotr who linked to the files on datafilehost, maybe he still has
them.

thx

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

In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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