[FFmpeg-devel] [PATCH 1/8] avcodec/cinepak: Require 1 bit per 4x4 block as minimum input

[Tomas Härdin]

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