| Message ID | 20190812191708.22608-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer: > Fixes: Timeout (12sec -> 32ms) > Fixes: 16078/clusterfuzz-testcase-minimized- > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/cinepak.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c > index aeb15de0ed..62eb794332 100644 > --- a/libavcodec/cinepak.c > +++ b/libavcodec/cinepak.c > @@ -356,6 +356,9 @@ static int cinepak_predecode_check > (CinepakContext *s) > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12) > return AVERROR_INVALIDDATA; > > + if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8)) > + return AVERROR_INVALIDDATA; This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You could merge it with the check above into something like: if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 + (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 : 0)) { return AVERROR_INVALIDDATA; } The check further down could also check each strip's size, not just the first one. Finally, I don't think we should accept files with num_strips > MAX_STRIPS in cinepak_decode(). We should ask for samples of them instead. /Tomas
ons 2019-08-14 klockan 12:32 +0200 skrev Tomas Härdin: > mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer: > > Fixes: Timeout (12sec -> 32ms) > > Fixes: 16078/clusterfuzz-testcase-minimized- > > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296 > > > > Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/cinepak.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c > > index aeb15de0ed..62eb794332 100644 > > --- a/libavcodec/cinepak.c > > +++ b/libavcodec/cinepak.c > > @@ -356,6 +356,9 @@ static int cinepak_predecode_check > > (CinepakContext *s) > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12) > > return AVERROR_INVALIDDATA; > > > > + if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8)) > > + return AVERROR_INVALIDDATA; > > This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You > could merge it with the check above into something like: > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 + > (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 : > 0)) { > return AVERROR_INVALIDDATA; > } > > The check further down could also check each strip's size, not just the > first one. Actually, thinking a bit more about this, I suspect it might be legal to have strips that don't cover the entire frame. It would certainly be good to support that, for optimizing skip frames. Not sure what old decoders make of that however. A skip could potentially be encoded in 22 + (width/4 + 7)/8 bytes while still being compatible. I'd replace s->avctx->height in the expression with the height of the first strip, to not constrain things too much. /Tomas
On Thu, Aug 15, 2019 at 04:43:19PM +0200, Tomas Härdin wrote: > ons 2019-08-14 klockan 12:32 +0200 skrev Tomas Härdin: > > mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer: > > > Fixes: Timeout (12sec -> 32ms) > > > Fixes: 16078/clusterfuzz-testcase-minimized- > > > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296 > > > > > > Found-by: continuous fuzzing process > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavcodec/cinepak.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c > > > index aeb15de0ed..62eb794332 100644 > > > --- a/libavcodec/cinepak.c > > > +++ b/libavcodec/cinepak.c > > > @@ -356,6 +356,9 @@ static int cinepak_predecode_check > > > (CinepakContext *s) > > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12) > > > return AVERROR_INVALIDDATA; > > > > > > + if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8)) > > > + return AVERROR_INVALIDDATA; > > > > This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You > > could merge it with the check above into something like: > > > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 + > > (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 : > > 0)) { > > return AVERROR_INVALIDDATA; > > } > > > > The check further down could also check each strip's size, not just the > > first one. > > Actually, thinking a bit more about this, I suspect it might be legal > to have strips that don't cover the entire frame. It would certainly be > good to support that, for optimizing skip frames. Not sure what old > decoders make of that however. A skip could potentially be encoded in > 22 + (width/4 + 7)/8 bytes while still being compatible. i was asking myself the same questions before writing the patch, and in the "spec" there is "Each frame is segmented into 4x4 pixel blocks, and each block is coded using either 1 or 4 vectors." "Each" meaning no holes to me. If thats actually true for all real files that i do not know. > > I'd replace s->avctx->height in the expression with the height of the > first strip, to not constrain things too much. ill post a patch doing that thx [...]
fre 2019-08-16 klockan 00:50 +0200 skrev Michael Niedermayer: > On Thu, Aug 15, 2019 at 04:43:19PM +0200, Tomas Härdin wrote: > > ons 2019-08-14 klockan 12:32 +0200 skrev Tomas Härdin: > > > mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer: > > > > Fixes: Timeout (12sec -> 32ms) > > > > Fixes: 16078/clusterfuzz-testcase-minimized- > > > > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296 > > > > [...] > > > > + if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8)) > > > > + return AVERROR_INVALIDDATA; > > > > > > This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You > > > could merge it with the check above into something like: > > > > > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 + > > > (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 : > > > 0)) { > > > return AVERROR_INVALIDDATA; > > > } > > > > > > The check further down could also check each strip's size, not just the > > > first one. > > > > Actually, thinking a bit more about this, I suspect it might be legal > > to have strips that don't cover the entire frame. It would certainly be > > good to support that, for optimizing skip frames. Not sure what old > > decoders make of that however. A skip could potentially be encoded in > > 22 + (width/4 + 7)/8 bytes while still being compatible. > > i was asking myself the same questions before writing the patch, and > in the "spec" there is > "Each frame is segmented into 4x4 pixel blocks, and each block is coded using either 1 or 4 vectors." > "Each" meaning no holes to me. If thats actually true for all real files > that i do not know. We should keep in mind the spec is fine with there being zero strips. It's only if one wants to support certain decoders that there will/must be one or more strips. I don't have any way of testing the MacOS decoder, but the old Win3.1 decoder is easy enough to get running. Then I can also check whether strips narrower than avctx->width are OK. > > I'd replace s->avctx->height in the expression with the height of the > > first strip, to not constrain things too much. > > ill post a patch doing that I feel like a better fix would be to make the decoder not choke on the kind of files that trigger this. With this style of check it's still possible to insert a second strip that triggers the slowness, with a trivially sized strip before it. Would you mind sending me the sample? /Tomas
fre 2019-08-16 klockan 14:57 +0200 skrev Tomas Härdin: > fre 2019-08-16 klockan 00:50 +0200 skrev Michael Niedermayer: > > On Thu, Aug 15, 2019 at 04:43:19PM +0200, Tomas Härdin wrote: > > > ons 2019-08-14 klockan 12:32 +0200 skrev Tomas Härdin: > > > > mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer: > > > > > Fixes: Timeout (12sec -> 32ms) > > > > > Fixes: 16078/clusterfuzz-testcase-minimized- > > > > > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296 > > > > > [...] > > > > > + if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8)) > > > > > + return AVERROR_INVALIDDATA; > > > > > > > > This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You > > > > could merge it with the check above into something like: > > > > > > > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 + > > > > (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 : > > > > 0)) { > > > > return AVERROR_INVALIDDATA; > > > > } > > > > > > > > The check further down could also check each strip's size, not just the > > > > first one. > > > > > > Actually, thinking a bit more about this, I suspect it might be legal > > > to have strips that don't cover the entire frame. It would certainly be > > > good to support that, for optimizing skip frames. Not sure what old > > > decoders make of that however. A skip could potentially be encoded in > > > 22 + (width/4 + 7)/8 bytes while still being compatible. > > > > i was asking myself the same questions before writing the patch, and > > in the "spec" there is > > "Each frame is segmented into 4x4 pixel blocks, and each block is coded using either 1 or 4 vectors." > > "Each" meaning no holes to me. If thats actually true for all real files > > that i do not know. > > We should keep in mind the spec is fine with there being zero strips. > It's only if one wants to support certain decoders that there will/must > be one or more strips. I've been playing around with the Windows 3.1 cinepak decoder, and it seems one indeed has to code every MB even if they don't change. I suspect the reason is because that decoder uses double buffering and they wanted to avoid doing more memcpy()s than absolutely necessary. If one tries to play tricks like coding strips that are only 4x4 pixels to indicate a frame without changes then parts of the video will be replaced by garbage. So demanding number_of_bits >= number_of_mbs is certainly in line with that decoder. I feel I should point out that being conservative here is at odds with the general "best effort" approach taken in this project. These toy codecs are useful as illustrative examples of this contradiction. I'm sure there are many more examples of files that can cause ffmpeg to do a lot more work than expected, for almost every codec. I know afl-fuzz is likely to find out that it can make the ZMBV decoder do a lot of work for a small input file, because I've seen it do that with gzip. The user base for cinepak is of course miniscule, so I doubt anyone's going to complain that their broken CVID files don't work any more. I certainly don't care since cinepakenc only puts out valid files. But again, for other formats we're just going to have to tell users to put ffmpeg inside a sandbox and stick a CPU limit on it. Even ffprobe is vulnerable to DoS-y things. /Tomas
On Fri, Aug 16, 2019 at 02:57:42PM +0200, Tomas Härdin wrote: > fre 2019-08-16 klockan 00:50 +0200 skrev Michael Niedermayer: > > On Thu, Aug 15, 2019 at 04:43:19PM +0200, Tomas Härdin wrote: > > > ons 2019-08-14 klockan 12:32 +0200 skrev Tomas Härdin: > > > > mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer: > > > > > Fixes: Timeout (12sec -> 32ms) > > > > > Fixes: 16078/clusterfuzz-testcase-minimized- > > > > > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296 > > > > > [...] > > > > > + if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8)) > > > > > + return AVERROR_INVALIDDATA; > > > > > > > > This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You > > > > could merge it with the check above into something like: > > > > > > > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 + > > > > (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 : > > > > 0)) { > > > > return AVERROR_INVALIDDATA; > > > > } > > > > > > > > The check further down could also check each strip's size, not just the > > > > first one. > > > > > > Actually, thinking a bit more about this, I suspect it might be legal > > > to have strips that don't cover the entire frame. It would certainly be > > > good to support that, for optimizing skip frames. Not sure what old > > > decoders make of that however. A skip could potentially be encoded in > > > 22 + (width/4 + 7)/8 bytes while still being compatible. > > > > i was asking myself the same questions before writing the patch, and > > in the "spec" there is > > "Each frame is segmented into 4x4 pixel blocks, and each block is coded using either 1 or 4 vectors." > > "Each" meaning no holes to me. If thats actually true for all real files > > that i do not know. > > We should keep in mind the spec is fine with there being zero strips. > It's only if one wants to support certain decoders that there will/must > be one or more strips. > > I don't have any way of testing the MacOS decoder, but the old Win3.1 > decoder is easy enough to get running. Then I can also check whether > strips narrower than avctx->width are OK. > > > > I'd replace s->avctx->height in the expression with the height of the > > > first strip, to not constrain things too much. > > > > ill post a patch doing that > > I feel like a better fix would be to make the decoder not choke on the > kind of files that trigger this. With this style of check it's still > possible to insert a second strip that triggers the slowness, with a > trivially sized strip before it. > > Would you mind sending me the sample? will send it privatly [...]
On Sat, Aug 17, 2019 at 12:26:27AM +0200, Tomas Härdin wrote: > fre 2019-08-16 klockan 14:57 +0200 skrev Tomas Härdin: > > fre 2019-08-16 klockan 00:50 +0200 skrev Michael Niedermayer: > > > On Thu, Aug 15, 2019 at 04:43:19PM +0200, Tomas Härdin wrote: > > > > ons 2019-08-14 klockan 12:32 +0200 skrev Tomas Härdin: > > > > > mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer: > > > > > > Fixes: Timeout (12sec -> 32ms) > > > > > > Fixes: 16078/clusterfuzz-testcase-minimized- > > > > > > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296 > > > > > > [...] > > > > > > + if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8)) > > > > > > + return AVERROR_INVALIDDATA; > > > > > > > > > > This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You > > > > > could merge it with the check above into something like: > > > > > > > > > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 + > > > > > (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 : > > > > > 0)) { > > > > > return AVERROR_INVALIDDATA; > > > > > } > > > > > > > > > > The check further down could also check each strip's size, not just the > > > > > first one. > > > > > > > > Actually, thinking a bit more about this, I suspect it might be legal > > > > to have strips that don't cover the entire frame. It would certainly be > > > > good to support that, for optimizing skip frames. Not sure what old > > > > decoders make of that however. A skip could potentially be encoded in > > > > 22 + (width/4 + 7)/8 bytes while still being compatible. > > > > > > i was asking myself the same questions before writing the patch, and > > > in the "spec" there is > > > "Each frame is segmented into 4x4 pixel blocks, and each block is coded using either 1 or 4 vectors." > > > "Each" meaning no holes to me. If thats actually true for all real files > > > that i do not know. > > > > We should keep in mind the spec is fine with there being zero strips. > > It's only if one wants to support certain decoders that there will/must > > be one or more strips. > > I've been playing around with the Windows 3.1 cinepak decoder, and it > seems one indeed has to code every MB even if they don't change. I > suspect the reason is because that decoder uses double buffering and > they wanted to avoid doing more memcpy()s than absolutely necessary. If > one tries to play tricks like coding strips that are only 4x4 pixels to > indicate a frame without changes then parts of the video will be > replaced by garbage. So demanding number_of_bits >= number_of_mbs is > certainly in line with that decoder. > > I feel I should point out that being conservative here is at odds with > the general "best effort" approach taken in this project. These toy > codecs are useful as illustrative examples of this contradiction. I'm > sure there are many more examples of files that can cause ffmpeg to do > a lot more work than expected, for almost every codec. I know afl-fuzz > is likely to find out that it can make the ZMBV decoder do a lot of > work for a small input file, because I've seen it do that with gzip. > > The user base for cinepak is of course miniscule, so I doubt anyone's > going to complain that their broken CVID files don't work any more. I > certainly don't care since cinepakenc only puts out valid files. > But > again, for other formats we're just going to have to tell users to put > ffmpeg inside a sandbox and stick a CPU limit on it. Even ffprobe is > vulnerable to DoS-y things. yes the question ATM is just what to do here about this codec ? apply the patch ? change it ? check the first slice as in: @@ -359,7 +359,16 @@ static int cinepak_predecode_check (CinepakContext *s) if (num_strips) { const uint8_t *data = s->data + 10 + s->sega_film_skip_bytes; int strip_size = AV_RB24 (data + 1); - if (strip_size < 12 || strip_size > encoded_buf_size) + + if (!(s->strips[0].y1 = AV_RB16 (&data[4]))) + s->strips[0].y2 = (s->strips[0].y1 = 0) + AV_RB16 (&data[8]); + else + s->strips[0].y2 = AV_RB16 (&data[8]); + + if (strip_size < 12 || strip_size > encoded_buf_size || + s->strips[0].y2 <= s->strips[0].y1 || + ((s->avctx->width * (int64_t)(s->strips[0].y2 - s->strips[0].y1)) / 16 + 7)/8 > s->size + ) return AVERROR_INVALIDDATA; } just raise the "threshold" in tools/target_dec_fuzzer.c for cinepak? something else ? This is not really a technical question, its a question what is preferred. thx [...]
lör 2019-08-17 klockan 17:33 +0200 skrev Michael Niedermayer: > On Sat, Aug 17, 2019 at 12:26:27AM +0200, Tomas Härdin wrote: > > fre 2019-08-16 klockan 14:57 +0200 skrev Tomas Härdin: > > > fre 2019-08-16 klockan 00:50 +0200 skrev Michael Niedermayer: > > > > On Thu, Aug 15, 2019 at 04:43:19PM +0200, Tomas Härdin wrote: > > > > > ons 2019-08-14 klockan 12:32 +0200 skrev Tomas Härdin: > > > > > > mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer: > > > > > > > Fixes: Timeout (12sec -> 32ms) > > > > > > > Fixes: 16078/clusterfuzz-testcase-minimized- > > > > > > > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296 > > > > > > > [...] > > > > > > > + if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8)) > > > > > > > + return AVERROR_INVALIDDATA; > > > > > > > > > > > > This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You > > > > > > could merge it with the check above into something like: > > > > > > > > > > > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 + > > > > > > (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 : > > > > > > 0)) { > > > > > > return AVERROR_INVALIDDATA; > > > > > > } > > > > > > > > > > > > The check further down could also check each strip's size, not just the > > > > > > first one. > > > > > > > > > > Actually, thinking a bit more about this, I suspect it might be legal > > > > > to have strips that don't cover the entire frame. It would certainly be > > > > > good to support that, for optimizing skip frames. Not sure what old > > > > > decoders make of that however. A skip could potentially be encoded in > > > > > 22 + (width/4 + 7)/8 bytes while still being compatible. > > > > > > > > i was asking myself the same questions before writing the patch, and > > > > in the "spec" there is > > > > "Each frame is segmented into 4x4 pixel blocks, and each block is coded using either 1 or 4 vectors." > > > > "Each" meaning no holes to me. If thats actually true for all real files > > > > that i do not know. > > > > > > We should keep in mind the spec is fine with there being zero strips. > > > It's only if one wants to support certain decoders that there will/must > > > be one or more strips. > > > > I've been playing around with the Windows 3.1 cinepak decoder, and it > > seems one indeed has to code every MB even if they don't change. I > > suspect the reason is because that decoder uses double buffering and > > they wanted to avoid doing more memcpy()s than absolutely necessary. If > > one tries to play tricks like coding strips that are only 4x4 pixels to > > indicate a frame without changes then parts of the video will be > > replaced by garbage. So demanding number_of_bits >= number_of_mbs is > > certainly in line with that decoder. > > > > I feel I should point out that being conservative here is at odds with > > the general "best effort" approach taken in this project. These toy > > codecs are useful as illustrative examples of this contradiction. I'm > > sure there are many more examples of files that can cause ffmpeg to do > > a lot more work than expected, for almost every codec. I know afl-fuzz > > is likely to find out that it can make the ZMBV decoder do a lot of > > work for a small input file, because I've seen it do that with gzip. > > > > The user base for cinepak is of course miniscule, so I doubt anyone's > > going to complain that their broken CVID files don't work any more. I > > certainly don't care since cinepakenc only puts out valid files. > > But > > again, for other formats we're just going to have to tell users to put > > ffmpeg inside a sandbox and stick a CPU limit on it. Even ffprobe is > > vulnerable to DoS-y things. > > yes > > the question ATM is just what to do here about this codec ? > apply the patch ? > change it ? Well for a start, the file is 65535 x 209 pixels, 3166 frames. I wouldn't call decoding that @ 263 fps particularly slow Second, it's not the decoder which is slow. If I comment out the "*got_frame = 1;" then the test also runs fast. I'm not sure what happens elsewhere with the decoded buffer, but I suspect there's a bunch of useless malloc()/memset()ing going on. Maybe the decoder is using ff_reget_buffer() or av_frame_ref() incorrectly, I'm not sure. As I said on IRC, this class of problems will exist for every codec. Cinepak is easy to decode, even at these resolutions. Just imagine what will happens when someone feeds in a 65535x209 av1 stream.. /Tomas
On Sun, Aug 18, 2019 at 02:35:45AM +0200, Tomas Härdin wrote: > lör 2019-08-17 klockan 17:33 +0200 skrev Michael Niedermayer: > > On Sat, Aug 17, 2019 at 12:26:27AM +0200, Tomas Härdin wrote: > > > fre 2019-08-16 klockan 14:57 +0200 skrev Tomas Härdin: > > > > fre 2019-08-16 klockan 00:50 +0200 skrev Michael Niedermayer: > > > > > On Thu, Aug 15, 2019 at 04:43:19PM +0200, Tomas Härdin wrote: > > > > > > ons 2019-08-14 klockan 12:32 +0200 skrev Tomas Härdin: > > > > > > > mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer: > > > > > > > > Fixes: Timeout (12sec -> 32ms) > > > > > > > > Fixes: 16078/clusterfuzz-testcase-minimized- > > > > > > > > ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296 > > > > > > > > [...] > > > > > > > > + if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8)) > > > > > > > > + return AVERROR_INVALIDDATA; > > > > > > > > > > > > > > This is wrong if num_strips == 0, and if the MB area is != 0 mod 8. You > > > > > > > could merge it with the check above into something like: > > > > > > > > > > > > > > if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12 + > > > > > > > (num_strips ? ((s->avctx->width * s->avctx->height) / 16 + 7)/8 : > > > > > > > 0)) { > > > > > > > return AVERROR_INVALIDDATA; > > > > > > > } > > > > > > > > > > > > > > The check further down could also check each strip's size, not just the > > > > > > > first one. > > > > > > > > > > > > Actually, thinking a bit more about this, I suspect it might be legal > > > > > > to have strips that don't cover the entire frame. It would certainly be > > > > > > good to support that, for optimizing skip frames. Not sure what old > > > > > > decoders make of that however. A skip could potentially be encoded in > > > > > > 22 + (width/4 + 7)/8 bytes while still being compatible. > > > > > > > > > > i was asking myself the same questions before writing the patch, and > > > > > in the "spec" there is > > > > > "Each frame is segmented into 4x4 pixel blocks, and each block is coded using either 1 or 4 vectors." > > > > > "Each" meaning no holes to me. If thats actually true for all real files > > > > > that i do not know. > > > > > > > > We should keep in mind the spec is fine with there being zero strips. > > > > It's only if one wants to support certain decoders that there will/must > > > > be one or more strips. > > > > > > I've been playing around with the Windows 3.1 cinepak decoder, and it > > > seems one indeed has to code every MB even if they don't change. I > > > suspect the reason is because that decoder uses double buffering and > > > they wanted to avoid doing more memcpy()s than absolutely necessary. If > > > one tries to play tricks like coding strips that are only 4x4 pixels to > > > indicate a frame without changes then parts of the video will be > > > replaced by garbage. So demanding number_of_bits >= number_of_mbs is > > > certainly in line with that decoder. > > > > > > I feel I should point out that being conservative here is at odds with > > > the general "best effort" approach taken in this project. These toy > > > codecs are useful as illustrative examples of this contradiction. I'm > > > sure there are many more examples of files that can cause ffmpeg to do > > > a lot more work than expected, for almost every codec. I know afl-fuzz > > > is likely to find out that it can make the ZMBV decoder do a lot of > > > work for a small input file, because I've seen it do that with gzip. > > > > > > The user base for cinepak is of course miniscule, so I doubt anyone's > > > going to complain that their broken CVID files don't work any more. I > > > certainly don't care since cinepakenc only puts out valid files. > > > But > > > again, for other formats we're just going to have to tell users to put > > > ffmpeg inside a sandbox and stick a CPU limit on it. Even ffprobe is > > > vulnerable to DoS-y things. > > > > yes > > > > the question ATM is just what to do here about this codec ? > > apply the patch ? > > change it ? > > Well for a start, the file is 65535 x 209 pixels, 3166 frames. I > wouldn't call decoding that @ 263 fps particularly slow > > Second, it's not the decoder which is slow. If I comment out the > "*got_frame = 1;" then the test also runs fast. I'm not sure what > happens elsewhere with the decoded buffer, but I suspect there's a > bunch of useless malloc()/memset()ing going on. Maybe the decoder is > using ff_reget_buffer() or av_frame_ref() incorrectly, I'm not sure. > > As I said on IRC, this class of problems will exist for every codec. > Cinepak is easy to decode, even at these resolutions. Just imagine what > will happens when someone feeds in a 65535x209 av1 stream.. i dont know about av1 specifically without checking the specs but with most of the mainstream codecs. areas not covered by slices are forbidden. So the case here of "nothing" wouldnt be allowed. also many codecs,especially mainstream ones from mpeg/itu have a maximum compression rate. some 16x16 block or other requiring a symbol or 2 to say its not the end of a slice and skiped. And the arithmetic or range coder or vlc coder having a moderate number of symbols per bit max. so feeding really crazy resolutions into a decoder requires some small but proportional amout of input bytes. double the width and the minimum input bytes double sort of. codecs OTOH which allow coding a whole frame in 10bytes input independant of the resolution behave worse in this way as that can produce orders of magnitude more load per bandwidth the attacker needs. Thanks [...]
diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c index aeb15de0ed..62eb794332 100644 --- a/libavcodec/cinepak.c +++ b/libavcodec/cinepak.c @@ -356,6 +356,9 @@ static int cinepak_predecode_check (CinepakContext *s) if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12) return AVERROR_INVALIDDATA; + if (s->size < (s->avctx->width * s->avctx->height) / (4*4*8)) + return AVERROR_INVALIDDATA; + if (num_strips) { const uint8_t *data = s->data + 10 + s->sega_film_skip_bytes; int strip_size = AV_RB24 (data + 1);
Fixes: Timeout (12sec -> 32ms) Fixes: 16078/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CINEPAK_fuzzer-5695832885559296 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/cinepak.c | 3 +++ 1 file changed, 3 insertions(+)