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

[Michael Niedermayer]

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

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200919/abef8793/attachment.sig>