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

[Michael Niedermayer]

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

ill post a updated patch

thx

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

"You are 36 times more likely to die in a bathtub than at the hands of a
terrorist. Also, you are 2.5 times more likely to become a president and
2 times more likely to become an astronaut, than to die in a terrorist
attack." -- Thoughty2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200317/bb72f62f/attachment.sig>