[FFmpeg-devel] [PATCH 1/4] avformat/asfdec_f: Fix overflow check in get_tag()

[Anton Khirnov]

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 at 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.

-- 
Anton Khirnov