diff mbox series

[FFmpeg-devel,7/9] avformat/id3v2: Check end for overflow in id3v2_parse()

Message ID 20210419182346.4445-7-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/9] avformat/utils: check dts/duration to be representable before using them
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Michael Niedermayer April 19, 2021, 6:23 p.m. UTC
Fixes: signed integer overflow: 9223372036840103978 + 67637280 cannot be represented in type 'long'
Fixes: 33341/clusterfuzz-testcase-minimized-ffmpeg_dem_DSF_fuzzer-6408154041679872

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/id3v2.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

James Almer April 22, 2021, 10:51 p.m. UTC | #1
On 4/19/2021 3:23 PM, Michael Niedermayer wrote:
> Fixes: signed integer overflow: 9223372036840103978 + 67637280 cannot be represented in type 'long'
> Fixes: 33341/clusterfuzz-testcase-minimized-ffmpeg_dem_DSF_fuzzer-6408154041679872
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>   libavformat/id3v2.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> index e0fef08789..0f7035d4c5 100644
> --- a/libavformat/id3v2.c
> +++ b/libavformat/id3v2.c
> @@ -824,7 +824,7 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata,
>       int isv34, unsync;
>       unsigned tlen;
>       char tag[5];
> -    int64_t next, end = avio_tell(pb) + len;
> +    int64_t next, end = avio_tell(pb);
>       int taghdrlen;
>       const char *reason = NULL;
>       AVIOContext pb_local;
> @@ -836,6 +836,10 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata,
>       av_unused int uncompressed_buffer_size = 0;
>       const char *comm_frame;
>   
> +    if (av_sat_add64(end, len) != end + (uint64_t)len)

Wouldn't a check like end > INT64_MAX - len be simpler?

> +        return;
> +    end += len;
> +
>       av_log(s, AV_LOG_DEBUG, "id3v2 ver:%d flags:%02X len:%d\n", version, flags, len);
>   
>       switch (version) {
> @@ -1057,7 +1061,7 @@ seek:
>   
>       /* Footer preset, always 10 bytes, skip over it */
>       if (version == 4 && flags & 0x10)
> -        end += 10;
> +        end = av_sat_add64(end, 10);
>   
>   error:
>       if (reason)
>
Michael Niedermayer April 23, 2021, 9:17 a.m. UTC | #2
On Thu, Apr 22, 2021 at 07:51:53PM -0300, James Almer wrote:
> On 4/19/2021 3:23 PM, Michael Niedermayer wrote:
> > Fixes: signed integer overflow: 9223372036840103978 + 67637280 cannot be represented in type 'long'
> > Fixes: 33341/clusterfuzz-testcase-minimized-ffmpeg_dem_DSF_fuzzer-6408154041679872
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >   libavformat/id3v2.c | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> > index e0fef08789..0f7035d4c5 100644
> > --- a/libavformat/id3v2.c
> > +++ b/libavformat/id3v2.c
> > @@ -824,7 +824,7 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata,
> >       int isv34, unsync;
> >       unsigned tlen;
> >       char tag[5];
> > -    int64_t next, end = avio_tell(pb) + len;
> > +    int64_t next, end = avio_tell(pb);
> >       int taghdrlen;
> >       const char *reason = NULL;
> >       AVIOContext pb_local;
> > @@ -836,6 +836,10 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata,
> >       av_unused int uncompressed_buffer_size = 0;
> >       const char *comm_frame;
> > +    if (av_sat_add64(end, len) != end + (uint64_t)len)
> 
> Wouldn't a check like end > INT64_MAX - len be simpler?

will change it so it does not need av_sat_add64() and will apply

thx

[...]
diff mbox series

Patch

diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
index e0fef08789..0f7035d4c5 100644
--- a/libavformat/id3v2.c
+++ b/libavformat/id3v2.c
@@ -824,7 +824,7 @@  static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata,
     int isv34, unsync;
     unsigned tlen;
     char tag[5];
-    int64_t next, end = avio_tell(pb) + len;
+    int64_t next, end = avio_tell(pb);
     int taghdrlen;
     const char *reason = NULL;
     AVIOContext pb_local;
@@ -836,6 +836,10 @@  static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata,
     av_unused int uncompressed_buffer_size = 0;
     const char *comm_frame;
 
+    if (av_sat_add64(end, len) != end + (uint64_t)len)
+        return;
+    end += len;
+
     av_log(s, AV_LOG_DEBUG, "id3v2 ver:%d flags:%02X len:%d\n", version, flags, len);
 
     switch (version) {
@@ -1057,7 +1061,7 @@  seek:
 
     /* Footer preset, always 10 bytes, skip over it */
     if (version == 4 && flags & 0x10)
-        end += 10;
+        end = av_sat_add64(end, 10);
 
 error:
     if (reason)