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
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
