| Message ID | 20200315212058.6324-1-michael@niedermayer.cc |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avformat/asfdec_f: Fix overflow check in get_tag() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Quoting Michael Niedermayer (2020-03-15 22:20:55) > Fixes: signed integer overflow: 2 * 1210064928 cannot be represented in type 'int' > Fixes: 20873/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5761116909338624 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/asfdec_f.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c > index 57dc3b09b9..3f19747d78 100644 > --- a/libavformat/asfdec_f.c > +++ b/libavformat/asfdec_f.c > @@ -321,7 +321,7 @@ static void get_tag(AVFormatContext *s, const char *key, int type, int len, int > int64_t off = avio_tell(s->pb); > #define LEN 22 > > - if ((unsigned)len >= (UINT_MAX - LEN) / 2) > + if ((unsigned)len >= (INT_MAX - LEN) / 2) Is the cast still necessary then?
On Mon, Mar 16, 2020 at 10:45:21AM +0100, Anton Khirnov wrote: > Quoting Michael Niedermayer (2020-03-15 22:20:55) > > Fixes: signed integer overflow: 2 * 1210064928 cannot be represented in type 'int' > > Fixes: 20873/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5761116909338624 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/asfdec_f.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c > > index 57dc3b09b9..3f19747d78 100644 > > --- a/libavformat/asfdec_f.c > > +++ b/libavformat/asfdec_f.c > > @@ -321,7 +321,7 @@ static void get_tag(AVFormatContext *s, const char *key, int type, int len, int > > int64_t off = avio_tell(s->pb); > > #define LEN 22 > > > > - if ((unsigned)len >= (UINT_MAX - LEN) / 2) > > + if ((unsigned)len >= (INT_MAX - LEN) / 2) > > Is the cast still necessary then? it only makes a difference for negative len. And negative len looks invalid and would cause problems. So some check for negative len is needed, the (unsigned) achievs this but i can replace it by an explicit len < 0 if people prefer ? thx [...]
Quoting Michael Niedermayer (2020-03-16 12:11:28) > On Mon, Mar 16, 2020 at 10:45:21AM +0100, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2020-03-15 22:20:55) > > > Fixes: signed integer overflow: 2 * 1210064928 cannot be represented in type 'int' > > > Fixes: 20873/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5761116909338624 > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavformat/asfdec_f.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c > > > index 57dc3b09b9..3f19747d78 100644 > > > --- a/libavformat/asfdec_f.c > > > +++ b/libavformat/asfdec_f.c > > > @@ -321,7 +321,7 @@ static void get_tag(AVFormatContext *s, const char *key, int type, int len, int > > > int64_t off = avio_tell(s->pb); > > > #define LEN 22 > > > > > > - if ((unsigned)len >= (UINT_MAX - LEN) / 2) > > > + if ((unsigned)len >= (INT_MAX - LEN) / 2) > > > > Is the cast still necessary then? > > it only makes a difference for negative len. And negative len looks invalid > and would cause problems. So some check for negative len is needed, the > (unsigned) achievs this but i can replace it by an explicit len < 0 > if people prefer ? I would say the length should be checked at the place where it is read and this function should assume len is sane. Looking at the code, the tag length is read as 32bit for: 1 metadata object 2 extended content description object 3 metadata library object 4 content encryption object For the first case, the spec limits the size to UINT16_MAX. For the other cases, I'd say UINT16_MAX is a reasonable limit for any sane file.
On Mon, Mar 16, 2020 at 12:32:09PM +0100, Anton Khirnov wrote: > Quoting Michael Niedermayer (2020-03-16 12:11:28) > > On Mon, Mar 16, 2020 at 10:45:21AM +0100, Anton Khirnov wrote: > > > Quoting Michael Niedermayer (2020-03-15 22:20:55) > > > > Fixes: signed integer overflow: 2 * 1210064928 cannot be represented in type 'int' > > > > Fixes: 20873/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5761116909338624 > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavformat/asfdec_f.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c > > > > index 57dc3b09b9..3f19747d78 100644 > > > > --- a/libavformat/asfdec_f.c > > > > +++ b/libavformat/asfdec_f.c > > > > @@ -321,7 +321,7 @@ static void get_tag(AVFormatContext *s, const char *key, int type, int len, int > > > > int64_t off = avio_tell(s->pb); > > > > #define LEN 22 > > > > > > > > - if ((unsigned)len >= (UINT_MAX - LEN) / 2) > > > > + if ((unsigned)len >= (INT_MAX - LEN) / 2) > > > > > > Is the cast still necessary then? > > > > it only makes a difference for negative len. And negative len looks invalid > > and would cause problems. So some check for negative len is needed, the > > (unsigned) achievs this but i can replace it by an explicit len < 0 > > if people prefer ? > > I would say the length should be checked at the place where it is read > and this function should assume len is sane. Looking at the code, the > tag length is read as 32bit for: > 1 metadata object > 2 extended content description object > 3 metadata library object > 4 content encryption object > > For the first case, the spec limits the size to UINT16_MAX. For the > other cases, I'd say UINT16_MAX is a reasonable limit for any sane file. ill post a updated patch thx [...]
diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c index 57dc3b09b9..3f19747d78 100644 --- a/libavformat/asfdec_f.c +++ b/libavformat/asfdec_f.c @@ -321,7 +321,7 @@ static void get_tag(AVFormatContext *s, const char *key, int type, int len, int int64_t off = avio_tell(s->pb); #define LEN 22 - if ((unsigned)len >= (UINT_MAX - LEN) / 2) + if ((unsigned)len >= (INT_MAX - LEN) / 2) return; if (!asf->export_xmp && !strncmp(key, "xmp", 3))
Fixes: signed integer overflow: 2 * 1210064928 cannot be represented in type 'int' Fixes: 20873/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5761116909338624 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/asfdec_f.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)