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

[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 ?
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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- 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/20190817/f90123d9/attachment.sig>