[FFmpeg-devel] [PATCH v4] avcodec/cfhd: More strictly check tag order and multiplicity

[Paul B Mahol]

On Sat, Sep 19, 2020 at 7:45 PM Michael Niedermayer <michael at niedermayer.cc>
wrote:

> On Mon, Aug 31, 2020 at 07:42:20PM +0200, Paul B Mahol wrote:
> > On 8/31/20, Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > On Mon, Aug 31, 2020 at 11:14:07AM +0200, Paul B Mahol wrote:
> > >> On 8/31/20, Michael Niedermayer <michael at niedermayer.cc> wrote:
> > >> > This is based on the encoder and a small number of CFHD sample files
> > >> > It should make the decoder more robust against crafted input.
> > >> > Due to the lack of a proper specification it is possible that this
> > >> > may be too strict and may need to be tuned as files not following
> this
> > >> > ordering are found.
> > >> >
> > >> > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > >> > ---
> > >>
> > >> I'm afraid this is not nice solution.
> > >> Please consider sharing samples that causes crash to me.
> > >> IMO I'm afraid that this patch is just band-aid and not proper
> solution.
> > >
> > > As this ended up discussed on IRC too, heres the relevant log as
> reference
> > > for the archieve as otherwise noone will find it
> > > an interleaved discussion about AAC with JEEB is removed
> > >
> > > <michaelni> durandal_1707, kierank, i dont have any crashing samples
> which
> > > are fixed by the cfhd table patch. That patch was intended to restrict
> > > what/where elements can occur so that for example the one specifying
> the
> > > subband number is in the subband "header" and not the plane "header"
> and
> > > also not twice and also not missing.
> > > <michaelni> Iam happy to implement this differently if you have a
> > > suggestion
> > > <durandal_1707> ah, this is patch to "fix" timeouts
> > > <michaelni> durandal_1707, why do you write such flaimbait replies? The
> > > patch has nothing to do with timeouts. Its rather that i see multiple
> fixes
> > > for out of array writes in the history of the CFHD decoder and its
> header
> > > parser allowing basically anything in any order no matter if it has any
> > > sense. And the patch tries to restrict this.
> > > <michaelni> if you are against the patch just say so and ill work on
> > > something else
> > > <michaelni> if work on this fron continues, the next logic step would
> be to
> > > ensure each band, plane and so on is coded exactly once
> > > <michaelni> because IIRC ATM the decoder doesnt check this either not
> just
> > > that any band related field could be in the plane or main header part
> and so
> > > on
> > > <j-b> I agree with michaelni here.
> > > <j-b> The tone is flamebait-y for no reason.
> > > <durandal_1707> michaelni: restricting that with ugly patch is not nice
> > >
> > > to continue this on the ML which is where patch review belongs
> > > which other way do you prefer ?
> > > It could be implemented in many differnt ways, i cannot read your mind
> ...
> > > Each method has some advanatges and disadvanatges from ease of
> > > supporting unexpected order in odd files to code complexity to ease of
> > > backporting the patch ...
> > >
> > > The whole loop could be redesigned and split up into 1 function for
> > > each header type (main / plane / band / ...) each such function would
> > > then have a similar loop with the subset of tags that can occur.
> > > if you prefer that ?
> > > but that would probably not be backported far, leaving some releases
> > > without this
> > > or as already said, i can just work on something else if people dont
> like
> > > this.
> > > Or maybe you want to implement this ? I can ofer to review your patch
> if
> > > you want to implement this instead.
> >
> > There are only some restrictions that can be tracked by reading SDK code.
> > I prefer minimal intrusion in code - the minimal restrictions possible.
>
> Iam not sure how to implement that in a way that works reliable and is not
> more code than the table uses and probably a partial rewrite of the parser.
>
> some elements like width / height related ones are mandatory they must be
> there, they must be there just once and they must be there for each
> "channel"
> that needs them.
> The fuzzer just now found a case where just a missing height triggers a
> segfault and we have a "minimal" test for height being  too small / not set
> already. The attacker just has too much freedom to structure the elements
> so tests dont work in this case it seems
>
> If there is generic code that ensures all mandatory elements occur than
> checking their values can be done when they are read.
> Without such code, i guess all checks would need to be seperated from the
> parsing as nothing in the parsing is guranteed to be run or the order in
> which
> its run.
> The alternative is to rewrite the parser so as to only read elements in the
> order they are supposed to occur and not a single loop parsing anything in
> any order
>

Please share a sample that causes crash privately.


>
> thx
>
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> it is not once nor twice but times without number that the same ideas make
> their appearance in the world. -- Aristotle
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".