diff mbox series

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

Message ID 20200816150816.19442-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel] avformat/utils: check for integer overflow in av_get_frame_filename2()
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Michael Niedermayer Aug. 16, 2020, 3:08 p.m. UTC
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@niedermayer.cc>
---
 libavformat/utils.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Paul B Mahol Aug. 16, 2020, 3:38 p.m. UTC | #1
On 8/16/20, Michael Niedermayer <michael@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@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?

> --
> 2.17.1
>
> _______________________________________________
> 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 Aug. 16, 2020, 4:01 p.m. UTC | #2
On Sun, Aug 16, 2020 at 05:38:29PM +0200, Paul B Mahol wrote:
> On 8/16/20, Michael Niedermayer <michael@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@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

thx

[...]
Paul B Mahol Aug. 16, 2020, 4:18 p.m. UTC | #3
On 8/16/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Sun, Aug 16, 2020 at 05:38:29PM +0200, Paul B Mahol wrote:
>> On 8/16/20, Michael Niedermayer <michael@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@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.

>
> thx
>
> [...]
> --
> 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
>
Michael Niedermayer Oct. 24, 2020, 3:14 p.m. UTC | #4
On Sun, Aug 16, 2020 at 06:18:02PM +0200, Paul B Mahol wrote:
> On 8/16/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Sun, Aug 16, 2020 at 05:38:29PM +0200, Paul B Mahol wrote:
> >> On 8/16/20, Michael Niedermayer <michael@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@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

[...]
diff mbox series

Patch

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));