diff mbox series

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

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()
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Michael Niedermayer March 15, 2020, 9:20 p.m. UTC
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(-)

Comments

Anton Khirnov March 16, 2020, 9:45 a.m. UTC | #1
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?
Michael Niedermayer March 16, 2020, 11:11 a.m. UTC | #2
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

[...]
Anton Khirnov March 16, 2020, 11:32 a.m. UTC | #3
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.
Michael Niedermayer March 16, 2020, 11:50 p.m. UTC | #4
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 mbox series

Patch

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