| Message ID | 20180916002738.7513-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
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 >
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 ? [...]
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?
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
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.
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 [...]
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 >
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;
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(-)