| Message ID | 20170816030442.31512-1-michael@niedermayer.cc |
|---|---|
| State | Superseded |
| Headers | show |
On 2017-08-16 05:04, Michael Niedermayer wrote: > Fixes: OOM > Fixes: 2710/clusterfuzz-testcase-minimized-4750001420894208 > > 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 | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c > index f126515bd1..861098a0f2 100644 > --- a/libavcodec/zmbv.c > +++ b/libavcodec/zmbv.c > @@ -589,8 +589,12 @@ static av_cold int decode_init(AVCodecContext *avctx) > // Needed if zlib unused or init aborted before inflateInit > memset(&c->zstream, 0, sizeof(z_stream)); > > - c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64); > + if ((avctx->width + 255ULL) * (avctx->height + 64ULL) > avctx->max_pixels) { Are width and height constrained somewhere? If both end up around 1<<32 then the multiplication can overflow. > + av_log(avctx, AV_LOG_ERROR, "Internal buffer larger than max_pixels\n"); > + return AVERROR_INVALIDDATA; > + } > > + c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64); Use 255ULL and 64ULL maybe? It's almost conceivable that you could have a file constructed to be (1<<31 - 255)x1 that passes the max_pixels check /Tomas
On Wed, Aug 16, 2017 at 09:48:43AM +0200, Tomas Härdin wrote: > On 2017-08-16 05:04, Michael Niedermayer wrote: > >Fixes: OOM > >Fixes: 2710/clusterfuzz-testcase-minimized-4750001420894208 > > > >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 | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > >diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c > >index f126515bd1..861098a0f2 100644 > >--- a/libavcodec/zmbv.c > >+++ b/libavcodec/zmbv.c > >@@ -589,8 +589,12 @@ static av_cold int decode_init(AVCodecContext *avctx) > > // Needed if zlib unused or init aborted before inflateInit > > memset(&c->zstream, 0, sizeof(z_stream)); > >- c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64); > >+ if ((avctx->width + 255ULL) * (avctx->height + 64ULL) > avctx->max_pixels) { > > Are width and height constrained somewhere? If both end up around > 1<<32 then the multiplication can overflow. width and height are signed integers and checked in avcodec_open2() I can replicate the check here if you think depending on a distant check is too fragile. > > >+ av_log(avctx, AV_LOG_ERROR, "Internal buffer larger than max_pixels\n"); > >+ return AVERROR_INVALIDDATA; > >+ } > >+ c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64); > > Use 255ULL and 64ULL maybe? It's almost conceivable that you could > have a file constructed to be (1<<31 - 255)x1 that passes the > max_pixels check such file could exist but avcodec_open2() will reject this currently. I think adding an explicit check of some kind would make sense 255ULL and 64ULL wont help as decomp_size is just a unsigned int and its passed into zlib which uses a uInt ... ill send a new patch thx [...]
On 2017-08-16 14:35, Michael Niedermayer wrote: > On Wed, Aug 16, 2017 at 09:48:43AM +0200, Tomas Härdin wrote: >> On 2017-08-16 05:04, Michael Niedermayer wrote: >>> Fixes: OOM >>> Fixes: 2710/clusterfuzz-testcase-minimized-4750001420894208 >>> >>> 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 | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c >>> index f126515bd1..861098a0f2 100644 >>> --- a/libavcodec/zmbv.c >>> +++ b/libavcodec/zmbv.c >>> @@ -589,8 +589,12 @@ static av_cold int decode_init(AVCodecContext *avctx) >>> // Needed if zlib unused or init aborted before inflateInit >>> memset(&c->zstream, 0, sizeof(z_stream)); >>> - c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64); >>> + if ((avctx->width + 255ULL) * (avctx->height + 64ULL) > avctx->max_pixels) { >> Are width and height constrained somewhere? If both end up around >> 1<<32 then the multiplication can overflow. > width and height are signed integers and checked in avcodec_open2() > > I can replicate the check here if you think depending on a distant > check is too fragile. Nah, so long as it's checked. Too bad C doesn't have range typed integers like Ada >>> + av_log(avctx, AV_LOG_ERROR, "Internal buffer larger than max_pixels\n"); >>> + return AVERROR_INVALIDDATA; >>> + } >>> + c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64); >> Use 255ULL and 64ULL maybe? It's almost conceivable that you could >> have a file constructed to be (1<<31 - 255)x1 that passes the >> max_pixels check > such file could exist but avcodec_open2() will reject this currently. > I think adding an explicit check of some kind would make sense > 255ULL and 64ULL wont help as decomp_size is just a unsigned int and > its passed into zlib which uses a uInt ... > > ill send a new patch > > thx Right, it might pass max_pixels but then overflow uInt.. /Tomas
diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c index f126515bd1..861098a0f2 100644 --- a/libavcodec/zmbv.c +++ b/libavcodec/zmbv.c @@ -589,8 +589,12 @@ static av_cold int decode_init(AVCodecContext *avctx) // Needed if zlib unused or init aborted before inflateInit memset(&c->zstream, 0, sizeof(z_stream)); - c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64); + if ((avctx->width + 255ULL) * (avctx->height + 64ULL) > avctx->max_pixels) { + av_log(avctx, AV_LOG_ERROR, "Internal buffer larger than max_pixels\n"); + return AVERROR_INVALIDDATA; + } + c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64); /* Allocate decompression buffer */ if (c->decomp_size) { if (!(c->decomp_buf = av_mallocz(c->decomp_size))) {
Fixes: OOM Fixes: 2710/clusterfuzz-testcase-minimized-4750001420894208 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 | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)