[FFmpeg-devel,2/2] avcodec/scpr: Check for min > max in decompress_p()

Submitted by Michael Niedermayer on Aug. 5, 2018, 1:16 a.m.

Details

Message ID 20180805011642.5290-2-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Aug. 5, 2018, 1:16 a.m.
Fixes: Timeout
Fixes: 9342/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SCPR_fuzzer-4795990841229312

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

Comments

Paul B Mahol Aug. 5, 2018, 8:08 a.m.
On 8/5/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> Fixes: Timeout
> Fixes:
> 9342/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SCPR_fuzzer-4795990841229312
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/scpr.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/libavcodec/scpr.c b/libavcodec/scpr.c
> index 72f59d5917..d1e47b09ac 100644
> --- a/libavcodec/scpr.c
> +++ b/libavcodec/scpr.c
> @@ -525,6 +525,9 @@ static int decompress_p(AVCodecContext *avctx,
>      if (ret < 0)
>          return ret;
>
> +    if (min > max)
> +        return AVERROR_INVALIDDATA;
> +

Shouldn't this check be actually bellow?
You sure this does not break valid files?

>      max += temp << 8;
>      memset(s->blocks, 0, sizeof(*s->blocks) * s->nbcount);
>
> --
> 2.18.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer Aug. 5, 2018, 9:46 p.m.
On Sun, Aug 05, 2018 at 10:08:31AM +0200, Paul B Mahol wrote:
> On 8/5/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > Fixes: Timeout
> > Fixes:
> > 9342/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SCPR_fuzzer-4795990841229312
> >
> > Found-by: continuous fuzzing process
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/scpr.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/libavcodec/scpr.c b/libavcodec/scpr.c
> > index 72f59d5917..d1e47b09ac 100644
> > --- a/libavcodec/scpr.c
> > +++ b/libavcodec/scpr.c
> > @@ -525,6 +525,9 @@ static int decompress_p(AVCodecContext *avctx,
> >      if (ret < 0)
> >          return ret;
> >
> > +    if (min > max)
> > +        return AVERROR_INVALIDDATA;
> > +
> 
> Shouldn't this check be actually bellow?

yes, fixed, locally


> You sure this does not break valid files?

i found no file that it breaks, beyond this, no iam not sure.
It mostly based on logic thinking that these would not be ordered the
other way, as that seems not usefull

Is there some specification or more files i can test ?


[...]
Paul B Mahol Aug. 6, 2018, 7:05 a.m.
On 8/5/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Sun, Aug 05, 2018 at 10:08:31AM +0200, Paul B Mahol wrote:
>> On 8/5/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> > Fixes: Timeout
>> > Fixes:
>> > 9342/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SCPR_fuzzer-4795990841229312
>> >
>> > Found-by: continuous fuzzing process
>> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> > ---
>> >  libavcodec/scpr.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/libavcodec/scpr.c b/libavcodec/scpr.c
>> > index 72f59d5917..d1e47b09ac 100644
>> > --- a/libavcodec/scpr.c
>> > +++ b/libavcodec/scpr.c
>> > @@ -525,6 +525,9 @@ static int decompress_p(AVCodecContext *avctx,
>> >      if (ret < 0)
>> >          return ret;
>> >
>> > +    if (min > max)
>> > +        return AVERROR_INVALIDDATA;
>> > +
>>
>> Shouldn't this check be actually bellow?
>
> yes, fixed, locally
>
>
>> You sure this does not break valid files?
>
> i found no file that it breaks, beyond this, no iam not sure.
> It mostly based on logic thinking that these would not be ordered the
> other way, as that seems not usefull
>
> Is there some specification or more files i can test ?

It should be fine.
Michael Niedermayer Aug. 15, 2018, 10:57 p.m.
On Mon, Aug 06, 2018 at 09:05:51AM +0200, Paul B Mahol wrote:
> On 8/5/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Sun, Aug 05, 2018 at 10:08:31AM +0200, Paul B Mahol wrote:
> >> On 8/5/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> > Fixes: Timeout
> >> > Fixes:
> >> > 9342/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SCPR_fuzzer-4795990841229312
> >> >
> >> > Found-by: continuous fuzzing process
> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> > ---
> >> >  libavcodec/scpr.c | 3 +++
> >> >  1 file changed, 3 insertions(+)
> >> >
> >> > diff --git a/libavcodec/scpr.c b/libavcodec/scpr.c
> >> > index 72f59d5917..d1e47b09ac 100644
> >> > --- a/libavcodec/scpr.c
> >> > +++ b/libavcodec/scpr.c
> >> > @@ -525,6 +525,9 @@ static int decompress_p(AVCodecContext *avctx,
> >> >      if (ret < 0)
> >> >          return ret;
> >> >
> >> > +    if (min > max)
> >> > +        return AVERROR_INVALIDDATA;
> >> > +
> >>
> >> Shouldn't this check be actually bellow?
> >
> > yes, fixed, locally
> >
> >
> >> You sure this does not break valid files?
> >
> > i found no file that it breaks, beyond this, no iam not sure.
> > It mostly based on logic thinking that these would not be ordered the
> > other way, as that seems not usefull
> >
> > Is there some specification or more files i can test ?
> 
> It should be fine.

will apply

thx

[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/scpr.c b/libavcodec/scpr.c
index 72f59d5917..d1e47b09ac 100644
--- a/libavcodec/scpr.c
+++ b/libavcodec/scpr.c
@@ -525,6 +525,9 @@  static int decompress_p(AVCodecContext *avctx,
     if (ret < 0)
         return ret;
 
+    if (min > max)
+        return AVERROR_INVALIDDATA;
+
     max += temp << 8;
     memset(s->blocks, 0, sizeof(*s->blocks) * s->nbcount);