| Message ID | 20200816150816.19442-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 03c479ce236955fc329c7f9f4765ee1ec256bb73 |
| Headers | show |
| Series | [FFmpeg-devel] avformat/utils: check for integer overflow in av_get_frame_filename2() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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".
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 [...]
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 >
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 --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));
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(-)