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

[Michael Niedermayer]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190818/c0db5d91/attachment.sig>