diff mbox series

[FFmpeg-devel,2/3] avformat/sbgdec: Check opt_duration and start for overflow

Message ID 20210624205724.18711-2-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/3] avformat/matroskadec: Reset state also on failure in matroska_reset_status()
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 June 24, 2021, 8:57 p.m. UTC
Fixes: signed integer overflow: 2788626175500000000 + 7118941284000000000 cannot be represented in type 'long'
Fixes: 35215/clusterfuzz-testcase-minimized-ffmpeg_dem_SBG_fuzzer-6123272247836672

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

Comments

James Almer June 25, 2021, 12:22 p.m. UTC | #1
On 6/24/2021 5:57 PM, Michael Niedermayer wrote:
> Fixes: signed integer overflow: 2788626175500000000 + 7118941284000000000 cannot be represented in type 'long'
> Fixes: 35215/clusterfuzz-testcase-minimized-ffmpeg_dem_SBG_fuzzer-6123272247836672
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>   libavformat/sbgdec.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/libavformat/sbgdec.c b/libavformat/sbgdec.c
> index dafdc4a1cc..0a6e927e57 100644
> --- a/libavformat/sbgdec.c
> +++ b/libavformat/sbgdec.c
> @@ -935,6 +935,9 @@ static int expand_timestamps(void *log, struct sbg_script *s)
>       }
>       if (s->start_ts == AV_NOPTS_VALUE)
>           s->start_ts = (s->opt_start_at_first && s->tseq) ? s->tseq[0].ts.t : now;
> +    if (av_sat_add64(s->start_ts, s->opt_duration) != s->start_ts + (uint64_t)s->opt_duration)

Can't this instead be an if (s->start_ts > INT64_MAX - s->opt_duration) 
check? Both s->start_ts and s->opt_duration are apparently guaranteed to 
be positive.

> +        return AVERROR_INVALIDDATA;
> +
>       s->end_ts = s->opt_duration ? s->start_ts + s->opt_duration :
>                   AV_NOPTS_VALUE; /* may be overridden later by -E option */
>       cur_ts = now;
>
Michael Niedermayer June 25, 2021, 3:58 p.m. UTC | #2
On Fri, Jun 25, 2021 at 09:22:03AM -0300, James Almer wrote:
> On 6/24/2021 5:57 PM, Michael Niedermayer wrote:
> > Fixes: signed integer overflow: 2788626175500000000 + 7118941284000000000 cannot be represented in type 'long'
> > Fixes: 35215/clusterfuzz-testcase-minimized-ffmpeg_dem_SBG_fuzzer-6123272247836672
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >   libavformat/sbgdec.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/libavformat/sbgdec.c b/libavformat/sbgdec.c
> > index dafdc4a1cc..0a6e927e57 100644
> > --- a/libavformat/sbgdec.c
> > +++ b/libavformat/sbgdec.c
> > @@ -935,6 +935,9 @@ static int expand_timestamps(void *log, struct sbg_script *s)
> >       }
> >       if (s->start_ts == AV_NOPTS_VALUE)
> >           s->start_ts = (s->opt_start_at_first && s->tseq) ? s->tseq[0].ts.t : now;
> > +    if (av_sat_add64(s->start_ts, s->opt_duration) != s->start_ts + (uint64_t)s->opt_duration)
> 
> Can't this instead be an if (s->start_ts > INT64_MAX - s->opt_duration)
> check? Both s->start_ts and s->opt_duration are apparently guaranteed to be
> positive.

The variables are read by str_to_time() which looks like it can read negative
numbers. 
But maybe iam missing something

thx

[...]
James Almer June 25, 2021, 8:32 p.m. UTC | #3
On 6/25/2021 12:58 PM, Michael Niedermayer wrote:
> On Fri, Jun 25, 2021 at 09:22:03AM -0300, James Almer wrote:
>> On 6/24/2021 5:57 PM, Michael Niedermayer wrote:
>>> Fixes: signed integer overflow: 2788626175500000000 + 7118941284000000000 cannot be represented in type 'long'
>>> Fixes: 35215/clusterfuzz-testcase-minimized-ffmpeg_dem_SBG_fuzzer-6123272247836672
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>    libavformat/sbgdec.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/libavformat/sbgdec.c b/libavformat/sbgdec.c
>>> index dafdc4a1cc..0a6e927e57 100644
>>> --- a/libavformat/sbgdec.c
>>> +++ b/libavformat/sbgdec.c
>>> @@ -935,6 +935,9 @@ static int expand_timestamps(void *log, struct sbg_script *s)
>>>        }
>>>        if (s->start_ts == AV_NOPTS_VALUE)
>>>            s->start_ts = (s->opt_start_at_first && s->tseq) ? s->tseq[0].ts.t : now;
>>> +    if (av_sat_add64(s->start_ts, s->opt_duration) != s->start_ts + (uint64_t)s->opt_duration)
>>
>> Can't this instead be an if (s->start_ts > INT64_MAX - s->opt_duration)
>> check? Both s->start_ts and s->opt_duration are apparently guaranteed to be
>> positive.
> 
> The variables are read by str_to_time() which looks like it can read negative
> numbers.

Afaics, it checks the very first character to be < '0' || > '9' for both 
hours and minutes, so strtol() is not going to see a '-'.
Is there supposed to be one for seconds in valid files? If not, the same 
check could be done and ensure no negative value is parsed.

> But maybe iam missing something
> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Nicolas George June 27, 2021, 6:22 p.m. UTC | #4
James Almer (12021-06-25):
> Afaics, it checks the very first character to be < '0' || > '9' for both
> hours and minutes, so strtol() is not going to see a '-'.
> Is there supposed to be one for seconds in valid files? If not, the same
> check could be done and ensure no negative value is parsed.

No, valid files cannot have negative times.

Also, I think with the time that adding this format was a misguided
idea. I suspect nobody uses it. We could just remove it.

Regards,
Michael Niedermayer June 28, 2021, 4:12 p.m. UTC | #5
On Sun, Jun 27, 2021 at 08:22:12PM +0200, Nicolas George wrote:
> James Almer (12021-06-25):
> > Afaics, it checks the very first character to be < '0' || > '9' for both
> > hours and minutes, so strtol() is not going to see a '-'.
> > Is there supposed to be one for seconds in valid files? If not, the same
> > check could be done and ensure no negative value is parsed.
> 
> No, valid files cannot have negative times.
> 
> Also, I think with the time that adding this format was a misguided
> idea. I suspect nobody uses it. We could just remove it.

maybe you could go over the parsing/reading of values in it and 
add appropriate checks for all. This would likely cut down on the
amount of integer issues, and someone like you who knows the
format well can likely do this faster than others like me fixing
one issue found by the fuzzers at a time

Thanks

[...]
Michael Niedermayer Sept. 14, 2021, 9:06 p.m. UTC | #6
On Fri, Jun 25, 2021 at 09:22:03AM -0300, James Almer wrote:
> On 6/24/2021 5:57 PM, Michael Niedermayer wrote:
> > Fixes: signed integer overflow: 2788626175500000000 + 7118941284000000000 cannot be represented in type 'long'
> > Fixes: 35215/clusterfuzz-testcase-minimized-ffmpeg_dem_SBG_fuzzer-6123272247836672
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >   libavformat/sbgdec.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/libavformat/sbgdec.c b/libavformat/sbgdec.c
> > index dafdc4a1cc..0a6e927e57 100644
> > --- a/libavformat/sbgdec.c
> > +++ b/libavformat/sbgdec.c
> > @@ -935,6 +935,9 @@ static int expand_timestamps(void *log, struct sbg_script *s)
> >       }
> >       if (s->start_ts == AV_NOPTS_VALUE)
> >           s->start_ts = (s->opt_start_at_first && s->tseq) ? s->tseq[0].ts.t : now;
> > +    if (av_sat_add64(s->start_ts, s->opt_duration) != s->start_ts + (uint64_t)s->opt_duration)
> 
> Can't this instead be an if (s->start_ts > INT64_MAX - s->opt_duration)
> check? Both s->start_ts and s->opt_duration are apparently guaranteed to be
> positive.

will apply with this variant

thx

[...]
diff mbox series

Patch

diff --git a/libavformat/sbgdec.c b/libavformat/sbgdec.c
index dafdc4a1cc..0a6e927e57 100644
--- a/libavformat/sbgdec.c
+++ b/libavformat/sbgdec.c
@@ -935,6 +935,9 @@  static int expand_timestamps(void *log, struct sbg_script *s)
     }
     if (s->start_ts == AV_NOPTS_VALUE)
         s->start_ts = (s->opt_start_at_first && s->tseq) ? s->tseq[0].ts.t : now;
+    if (av_sat_add64(s->start_ts, s->opt_duration) != s->start_ts + (uint64_t)s->opt_duration)
+        return AVERROR_INVALIDDATA;
+
     s->end_ts = s->opt_duration ? s->start_ts + s->opt_duration :
                 AV_NOPTS_VALUE; /* may be overridden later by -E option */
     cur_ts = now;