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

[Michael Niedermayer]

On Sat, Sep 19, 2020 at 07:47:49PM +0200, Paul B Mahol wrote:
> 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.

sorry for the delay, i had some problems with making this reproduce reliably
i had this reproducing with a modified ffmpeg but it seems i got it working
now with unmodified.

ill send the file and command line which produces this backtrace privately 

but i think this decoder has more issues than just this one.

[cfhd @ 0x16bbb000] Height 2601
[cfhd @ 0x16bbb000] Width 13
[cfhd @ 0x16bbb000] Unknown tag 4 data 1a4a
[cfhd @ 0x16bbb000] Unknown tag 1115 data 1154
[cfhd @ 0x16bbb000] Unknown tag 4 data f0f
[cfhd @ 0x16bbb000] Start of lowpass coeffs component 0 height:3, width:24
[cfhd @ 0x16bbb000] Lowpass coefficients 72
[cfhd @ 0x16bbb000] Unknown tag 0 data 0
    Last message repeated 36 times
[cfhd @ 0x16bbb000] Unknown tag 16711 data 15
[cfhd @ 0x16bbb000] Small chunk length 80 required
[cfhd @ 0x16bbb000] Decoding level 1 plane 0 3 24 24
[cfhd @ 0x16bbb000] Level 2 plane 0 0 24 24
==2071== Invalid read of size 2
==2071==    at 0x7EF018: filter (cfhddsp.c:57)
==2071==    by 0x7EF206: vert_filter (cfhddsp.c:74)
==2071==    by 0x7EB31C: cfhd_decode (cfhd.c:958)
==2071==    by 0x818243: decode_simple_internal (decode.c:352)
==2071==    by 0x818E40: decode_simple_receive_frame (decode.c:549)
==2071==    by 0x818F26: decode_receive_frame_internal (decode.c:569)
==2071==    by 0x819183: avcodec_send_packet (decode.c:627)
==2071==    by 0x24EDED: decode (ffmpeg.c:2218)
==2071==    by 0x24F69D: decode_video (ffmpeg.c:2360)
==2071==    by 0x250791: process_input_packet (ffmpeg.c:2601)
==2071==    by 0x25812C: process_input (ffmpeg.c:4494)
==2071==    by 0x2586ED: transcode_step (ffmpeg.c:4614)
==2071==    by 0x25886A: transcode (ffmpeg.c:4668)
==2071==    by 0x2591D4: main (ffmpeg.c:4873)
==2071==  Address 0x17fc4f60 is 1,523,296 bytes inside an unallocated block of size 1,523,424 in arena "client"


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- 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/37280c39/attachment.sig>