[FFmpeg-devel] [PATCH] avformat/utils: check for integer overflow in av_get_frame_filename2()

[Michael Niedermayer]

On Sun, Aug 16, 2020 at 06:18:02PM +0200, Paul B Mahol wrote:
> On 8/16/20, Michael Niedermayer <michael at niedermayer.cc> wrote:
> > On Sun, Aug 16, 2020 at 05:38:29PM +0200, Paul B Mahol wrote:
> >> On 8/16/20, Michael Niedermayer <michael at niedermayer.cc> wrote:
> >> > Fixes: signed integer overflow: 317316873 * 10 cannot be represented in
> >> > type
> >> > 'int'
> >> > Fixes:
> >> > 24708/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5731180885049344
> >> >
> >> > Found-by: continuous fuzzing process
> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >> > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> >> > ---
> >> >  libavformat/utils.c | 5 ++++-
> >> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/libavformat/utils.c b/libavformat/utils.c
> >> > index 807d9f10cb..60e6fe5be5 100644
> >> > --- a/libavformat/utils.c
> >> > +++ b/libavformat/utils.c
> >> > @@ -4745,8 +4745,11 @@ int av_get_frame_filename2(char *buf, int
> >> > buf_size,
> >> > const char *path, int number
> >> >          if (c == '%') {
> >> >              do {
> >> >                  nd = 0;
> >> > -                while (av_isdigit(*p))
> >> > +                while (av_isdigit(*p)) {
> >> > +                    if (nd >= INT_MAX / 10 - 255)
> >> > +                        goto fail;
> >> >                      nd = nd * 10 + *p++ - '0';
> >> > +                }
> >> >                  c = *p++;
> >> >              } while (av_isdigit(c));
> >> >
> >>
> >> Why to limit it?
> >> Use int64_t?
> >
> > The buffer is only 20 bytes long, INT_MAX is already a few million times
> > more than what would work
> 
> Buffer size is wrong.

the patch would not change with a larger buffer. What the patch is checking
is INT_MAX the buffer is 20 bytes

nd stands probably for number of digits. we fail when we hit 2 billion digits
thats way beyond what any real case uses. Filenames are not billions of bytes
long. a int64_t also makes no sense for this.
This is NOT about the number of files or file index this is the number of
digits in which that index is represented.

so, i intend to apply this.
if you want to increase the 20 byte buffer, feel free to do so. But thats
unrelated to the patch. Also the public API limits the index to 32bit which
too isnt ideal ...

Thanks

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

Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20201024/69527848/attachment.sig>