[FFmpeg-devel,2/2] avcodec/zmbv: Check that the raw input is large enough to contain MVs or an intra frame

Submitted by Michael Niedermayer on Sept. 16, 2018, 12:27 a.m.

Details

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

Commit Message

Michael Niedermayer Sept. 16, 2018, 12:27 a.m.
Fixes: Timeout
Fixes: 10182/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ZMBV_fuzzer-6245951174344704

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

Comments

Paul B Mahol Sept. 16, 2018, 8:16 a.m.
On 9/16/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> Fixes: Timeout
> Fixes:
> 10182/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ZMBV_fuzzer-6245951174344704
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/zmbv.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c
> index 9e27a2caad..1133bdf7ba 100644
> --- a/libavcodec/zmbv.c
> +++ b/libavcodec/zmbv.c
> @@ -409,6 +409,7 @@ static int decode_frame(AVCodecContext *avctx, void
> *data, int *got_frame, AVPac
>      int zret = Z_OK; // Zlib return code
>      int len = buf_size;
>      int hi_ver, lo_ver, ret;
> +    int min_size;
>
>      /* parse header */
>      if (len < 1)
> @@ -510,7 +511,11 @@ static int decode_frame(AVCodecContext *avctx, void
> *data, int *got_frame, AVPac
>          memset(c->prev, 0, avctx->width * avctx->height * (c->bpp / 8));
>          c->decode_intra= decode_intra;
>      }
> -
> +    if (c->flags & ZMBV_KEYFRAME) {
> +        min_size = avctx->width * avctx->height * (c->bpp / 8);

This is pure guessing?

> +    } else {
> +        min_size = (c->bx * c->by * 2 + 3) & ~3;
> +    }
>      if (!c->decode_intra) {
>          av_log(avctx, AV_LOG_ERROR, "Error! Got no format or no
> keyframe!\n");
>          return AVERROR_INVALIDDATA;
> @@ -524,6 +529,10 @@ static int decode_frame(AVCodecContext *avctx, void
> *data, int *got_frame, AVPac
>              av_log(avctx, AV_LOG_ERROR, "Buffer too small\n");
>              return AVERROR_INVALIDDATA;
>          }
> +        if (min_size > len) {
> +            av_log(avctx, AV_LOG_ERROR, "input too small\n");
> +            return AVERROR_INVALIDDATA;
> +        }
>          memcpy(c->decomp_buf, buf, len);
>      } else { // ZLIB-compressed data
>          c->zstream.total_in = c->zstream.total_out = 0;
> --
> 2.18.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer Sept. 16, 2018, 11:47 p.m.
On Sun, Sep 16, 2018 at 10:16:05AM +0200, Paul B Mahol wrote:
> On 9/16/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > Fixes: Timeout
> > Fixes:
> > 10182/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ZMBV_fuzzer-6245951174344704
> >
> > Found-by: continuous fuzzing process
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/zmbv.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c
> > index 9e27a2caad..1133bdf7ba 100644
> > --- a/libavcodec/zmbv.c
> > +++ b/libavcodec/zmbv.c
> > @@ -409,6 +409,7 @@ static int decode_frame(AVCodecContext *avctx, void
> > *data, int *got_frame, AVPac
> >      int zret = Z_OK; // Zlib return code
> >      int len = buf_size;
> >      int hi_ver, lo_ver, ret;
> > +    int min_size;
> >
> >      /* parse header */
> >      if (len < 1)
> > @@ -510,7 +511,11 @@ static int decode_frame(AVCodecContext *avctx, void
> > *data, int *got_frame, AVPac
> >          memset(c->prev, 0, avctx->width * avctx->height * (c->bpp / 8));
> >          c->decode_intra= decode_intra;
> >      }
> > -
> > +    if (c->flags & ZMBV_KEYFRAME) {
> > +        min_size = avctx->width * avctx->height * (c->bpp / 8);
> 
> This is pure guessing?

theres a bit of logic behind it but ultimatly yes, its guessing

The logic
If the input is smaller for a raw keyframe the buffer would not be
fully updated. 
If the buffer is not fully updated then this should be a inter frame
not a keyframe.

The 2nd part, is that all files i found 
(http://samples.mplayerhq.hu/V-codecs/ZMBV/)
follow that assumtation

The 3rd is that the source code for zmbv i could fine (dosbox)
seems to never write uncompressed keyframes.

The 4th is that the multimedia wiki doesnt seem to say anything about the
uncompressed keyframe size

so i thought its better to use this as the minimal size and avoid the
issue the fuzzer found.

Its quite possible that i have missed something.
Also we could ask for a sample in the failure case or do something else if you
have some other suggestion ?



[...]
Paul B Mahol Sept. 17, 2018, 7:58 a.m.
On 9/17/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Sun, Sep 16, 2018 at 10:16:05AM +0200, Paul B Mahol wrote:
>> On 9/16/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> > Fixes: Timeout
>> > Fixes:
>> > 10182/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ZMBV_fuzzer-6245951174344704
>> >
>> > Found-by: continuous fuzzing process
>> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> > ---
>> >  libavcodec/zmbv.c | 11 ++++++++++-
>> >  1 file changed, 10 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c
>> > index 9e27a2caad..1133bdf7ba 100644
>> > --- a/libavcodec/zmbv.c
>> > +++ b/libavcodec/zmbv.c
>> > @@ -409,6 +409,7 @@ static int decode_frame(AVCodecContext *avctx, void
>> > *data, int *got_frame, AVPac
>> >      int zret = Z_OK; // Zlib return code
>> >      int len = buf_size;
>> >      int hi_ver, lo_ver, ret;
>> > +    int min_size;
>> >
>> >      /* parse header */
>> >      if (len < 1)
>> > @@ -510,7 +511,11 @@ static int decode_frame(AVCodecContext *avctx,
>> > void
>> > *data, int *got_frame, AVPac
>> >          memset(c->prev, 0, avctx->width * avctx->height * (c->bpp /
>> > 8));
>> >          c->decode_intra= decode_intra;
>> >      }
>> > -
>> > +    if (c->flags & ZMBV_KEYFRAME) {
>> > +        min_size = avctx->width * avctx->height * (c->bpp / 8);
>>
>> This is pure guessing?
>
> theres a bit of logic behind it but ultimatly yes, its guessing
>
> The logic
> If the input is smaller for a raw keyframe the buffer would not be
> fully updated.
> If the buffer is not fully updated then this should be a inter frame
> not a keyframe.
>
> The 2nd part, is that all files i found
> (http://samples.mplayerhq.hu/V-codecs/ZMBV/)
> follow that assumtation
>
> The 3rd is that the source code for zmbv i could fine (dosbox)
> seems to never write uncompressed keyframes.
>
> The 4th is that the multimedia wiki doesnt seem to say anything about the
> uncompressed keyframe size
>
> so i thought its better to use this as the minimal size and avoid the
> issue the fuzzer found.
>
> Its quite possible that i have missed something.
> Also we could ask for a sample in the failure case or do something else if
> you
> have some other suggestion ?

Yes, drop patch and leave code as is.

Also why this codec needs fuzzing?
Carl Eugen Hoyos Sept. 17, 2018, 11:45 a.m.
2018-09-17 9:58 GMT+02:00, Paul B Mahol <onemda@gmail.com>:
> Also why this codec needs fuzzing?

Do you believe that this codec is a less likely attack vector
than any other codec?

Carl Eugen
Paul B Mahol Sept. 17, 2018, 11:48 a.m.
On 9/17/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 2018-09-17 9:58 GMT+02:00, Paul B Mahol <onemda@gmail.com>:
>> Also why this codec needs fuzzing?
>
> Do you believe that this codec is a less likely attack vector
> than any other codec?

It is not possible attack vector at al, becuase there are no
security bugs in this codec that can be fixed in its code.
Michael Niedermayer Sept. 17, 2018, 6:27 p.m.
On Mon, Sep 17, 2018 at 09:58:18AM +0200, Paul B Mahol wrote:
> On 9/17/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Sun, Sep 16, 2018 at 10:16:05AM +0200, Paul B Mahol wrote:
> >> On 9/16/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> > Fixes: Timeout
> >> > Fixes:
> >> > 10182/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ZMBV_fuzzer-6245951174344704
> >> >
> >> > Found-by: continuous fuzzing process
> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> > ---
> >> >  libavcodec/zmbv.c | 11 ++++++++++-
> >> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c
> >> > index 9e27a2caad..1133bdf7ba 100644
> >> > --- a/libavcodec/zmbv.c
> >> > +++ b/libavcodec/zmbv.c
> >> > @@ -409,6 +409,7 @@ static int decode_frame(AVCodecContext *avctx, void
> >> > *data, int *got_frame, AVPac
> >> >      int zret = Z_OK; // Zlib return code
> >> >      int len = buf_size;
> >> >      int hi_ver, lo_ver, ret;
> >> > +    int min_size;
> >> >
> >> >      /* parse header */
> >> >      if (len < 1)
> >> > @@ -510,7 +511,11 @@ static int decode_frame(AVCodecContext *avctx,
> >> > void
> >> > *data, int *got_frame, AVPac
> >> >          memset(c->prev, 0, avctx->width * avctx->height * (c->bpp /
> >> > 8));
> >> >          c->decode_intra= decode_intra;
> >> >      }
> >> > -
> >> > +    if (c->flags & ZMBV_KEYFRAME) {
> >> > +        min_size = avctx->width * avctx->height * (c->bpp / 8);
> >>
> >> This is pure guessing?
> >
> > theres a bit of logic behind it but ultimatly yes, its guessing
> >
> > The logic
> > If the input is smaller for a raw keyframe the buffer would not be
> > fully updated.
> > If the buffer is not fully updated then this should be a inter frame
> > not a keyframe.
> >
> > The 2nd part, is that all files i found
> > (http://samples.mplayerhq.hu/V-codecs/ZMBV/)
> > follow that assumtation
> >
> > The 3rd is that the source code for zmbv i could fine (dosbox)
> > seems to never write uncompressed keyframes.
> >
> > The 4th is that the multimedia wiki doesnt seem to say anything about the
> > uncompressed keyframe size
> >
> > so i thought its better to use this as the minimal size and avoid the
> > issue the fuzzer found.
> >
> > Its quite possible that i have missed something.
> > Also we could ask for a sample in the failure case or do something else if
> > you
> > have some other suggestion ?
> 

> Yes, drop patch and leave code as is.

I would prefer to fix the issue in some form. Do you have some suggestion
or preferrance about how to fix this issue ?


> 
> Also why this codec needs fuzzing?

it needs fuzzing as much or as little as any other codec. Theres
nothing special on it

Also as i was looking over this earlier today, i could improve the check
by also including compressed frames. And i found a kind of unrelated
bug in the codec. Ill resend the related parts of the patchset

Thanks


[...]
Paul B Mahol Sept. 17, 2018, 7:06 p.m.
On 9/17/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Mon, Sep 17, 2018 at 09:58:18AM +0200, Paul B Mahol wrote:
>> On 9/17/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> > On Sun, Sep 16, 2018 at 10:16:05AM +0200, Paul B Mahol wrote:
>> >> On 9/16/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> >> > Fixes: Timeout
>> >> > Fixes:
>> >> > 10182/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ZMBV_fuzzer-6245951174344704
>> >> >
>> >> > Found-by: continuous fuzzing process
>> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> >> > ---
>> >> >  libavcodec/zmbv.c | 11 ++++++++++-
>> >> >  1 file changed, 10 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c
>> >> > index 9e27a2caad..1133bdf7ba 100644
>> >> > --- a/libavcodec/zmbv.c
>> >> > +++ b/libavcodec/zmbv.c
>> >> > @@ -409,6 +409,7 @@ static int decode_frame(AVCodecContext *avctx,
>> >> > void
>> >> > *data, int *got_frame, AVPac
>> >> >      int zret = Z_OK; // Zlib return code
>> >> >      int len = buf_size;
>> >> >      int hi_ver, lo_ver, ret;
>> >> > +    int min_size;
>> >> >
>> >> >      /* parse header */
>> >> >      if (len < 1)
>> >> > @@ -510,7 +511,11 @@ static int decode_frame(AVCodecContext *avctx,
>> >> > void
>> >> > *data, int *got_frame, AVPac
>> >> >          memset(c->prev, 0, avctx->width * avctx->height * (c->bpp /
>> >> > 8));
>> >> >          c->decode_intra= decode_intra;
>> >> >      }
>> >> > -
>> >> > +    if (c->flags & ZMBV_KEYFRAME) {
>> >> > +        min_size = avctx->width * avctx->height * (c->bpp / 8);
>> >>
>> >> This is pure guessing?
>> >
>> > theres a bit of logic behind it but ultimatly yes, its guessing
>> >
>> > The logic
>> > If the input is smaller for a raw keyframe the buffer would not be
>> > fully updated.
>> > If the buffer is not fully updated then this should be a inter frame
>> > not a keyframe.
>> >
>> > The 2nd part, is that all files i found
>> > (http://samples.mplayerhq.hu/V-codecs/ZMBV/)
>> > follow that assumtation
>> >
>> > The 3rd is that the source code for zmbv i could fine (dosbox)
>> > seems to never write uncompressed keyframes.
>> >
>> > The 4th is that the multimedia wiki doesnt seem to say anything about
>> > the
>> > uncompressed keyframe size
>> >
>> > so i thought its better to use this as the minimal size and avoid the
>> > issue the fuzzer found.
>> >
>> > Its quite possible that i have missed something.
>> > Also we could ask for a sample in the failure case or do something else
>> > if
>> > you
>> > have some other suggestion ?
>>
>
>> Yes, drop patch and leave code as is.
>
> I would prefer to fix the issue in some form. Do you have some suggestion
> or preferrance about how to fix this issue ?

No, you are not interested in fixing real issues.
Please do not commit hacks like this patch.

Nothing in zlib guarantees that size will fit under your random constraints.

>
>
>>
>> Also why this codec needs fuzzing?
>
> it needs fuzzing as much or as little as any other codec. Theres
> nothing special on it
>
> Also as i was looking over this earlier today, i could improve the check
> by also including compressed frames. And i found a kind of unrelated
> bug in the codec. Ill resend the related parts of the patchset
>
> Thanks
>
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The smallest minority on earth is the individual. Those who deny
> individual rights cannot claim to be defenders of minorities. - Ayn Rand
>

Patch hide | download patch | download mbox

diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c
index 9e27a2caad..1133bdf7ba 100644
--- a/libavcodec/zmbv.c
+++ b/libavcodec/zmbv.c
@@ -409,6 +409,7 @@  static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, AVPac
     int zret = Z_OK; // Zlib return code
     int len = buf_size;
     int hi_ver, lo_ver, ret;
+    int min_size;
 
     /* parse header */
     if (len < 1)
@@ -510,7 +511,11 @@  static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, AVPac
         memset(c->prev, 0, avctx->width * avctx->height * (c->bpp / 8));
         c->decode_intra= decode_intra;
     }
-
+    if (c->flags & ZMBV_KEYFRAME) {
+        min_size = avctx->width * avctx->height * (c->bpp / 8);
+    } else {
+        min_size = (c->bx * c->by * 2 + 3) & ~3;
+    }
     if (!c->decode_intra) {
         av_log(avctx, AV_LOG_ERROR, "Error! Got no format or no keyframe!\n");
         return AVERROR_INVALIDDATA;
@@ -524,6 +529,10 @@  static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, AVPac
             av_log(avctx, AV_LOG_ERROR, "Buffer too small\n");
             return AVERROR_INVALIDDATA;
         }
+        if (min_size > len) {
+            av_log(avctx, AV_LOG_ERROR, "input too small\n");
+            return AVERROR_INVALIDDATA;
+        }
         memcpy(c->decomp_buf, buf, len);
     } else { // ZLIB-compressed data
         c->zstream.total_in = c->zstream.total_out = 0;