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

[Tomas Härdin]

sön 2019-08-18 klockan 12:19 +0200 skrev Michael Niedermayer:
> On Sun, Aug 18, 2019 at 12:00:45PM +0200, Paul B Mahol wrote:
> > On Sun, Aug 18, 2019 at 11:44 AM Michael Niedermayer <michael at niedermayer.cc>
> > wrote:
> > 
> > > On Sun, Aug 18, 2019 at 10:47:26AM +0200, Tomas Härdin wrote:
> > > > sön 2019-08-18 klockan 02:35 +0200 skrev Tomas Härdin:
> > > > 
> > > > I did some investigation, it is indeed ff_reget_buffer(). It copies the
> > > > frame data for some reason. The fix is simple in this case: just call
> > > > ff_get_buffer() once in cinepak_decode_init() and keep overwriting the
> > > > same frame.
> > > > 
> > > > > 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..
> > > > 
> > > > And related to this, ff_reget_buffer() is used for a lot of these
> > > > codecs which only overwrite pixels in the old frame. flicvideo, gifdec,
> > > > msrle, roqvideodec and others probably have the same flaw.
> > > 
> > > not calling any form of *get_buffer per frame breaks decoding into
> > > user supplied buffers.
> > > 
> > > If you check the documentation of the get_buffer2 callback
> > > 
> > > " This callback is called at the beginning of each frame to get data
> > > buffer(s) for it."
> > > 
> > > That would not be possible if its just called once in init

Sorry, I'm a bit rusty on lavc internals.

> > > and yes i too wish there was a magic fix but i think most things that
> > > look like magic fixes have a fatal flaw. But maybe iam missing something
> > > in fact i hope that iam missing something and that there is a magic fix
> > > 
> > 
> > Magic fix is enabling reference counted frames in fuzzer.
> 
> That is covered by the part below which you maybe did not read
> 
> > > PS: if you think of changing the API, i dont think its the API.
> > > I mean every user application will read the frames it receives, so
> > > even if inside the decoder we just return the same frame with 2 pixels
> > > different the user doesnt know this and has to read the whole frame.
> > > The problem is moved around but its still there.

Copying is still slower than not copying. Enabling refcounting fixes
the timeout issue here, and will likely silence a whole bunch of false
positives for this class of files.

It would be beneficial to have a consistent way of signalling that a
frame didn't change, since a bunch of codecs have that property.
Currently it's a mix of discontinuous timestamps, longer frame
durations and repeating identical frames.

> 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.

Lavc is already very lenient with what it accepts. How do you detect
the difference between "this packet is too small to decode to an entire
frame" from "this packet is too small but we could still get a few MBs
out of it"?

FTR I've been in favor of rejecting broken files for a while now, and
I'm in favor of something like checking that the number of bytes in all
strips add up to some minimum. This can be combined with a pre-parsing
step that also rejects the input if any chunk has incorrect size.

> 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.

Users are ultimately responsible for limiting how much resources their
programs can use. As you say, getting high amplification isn't hard.
Not even ffprobe is safe from this. I've been in talks with the
peertube people about this very topic not too long ago.

Stuff like this is why I've been harping on about parsers and verifying
things, and being very specific about what we accept. Unfortunately
FFculture seems to be against this.

/Tomas