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 <mich...@niedermayer.cc> > wrote: > > > On Mon, Aug 31, 2020 at 07:42:20PM +0200, Paul B Mahol wrote: > > > On 8/31/20, Michael Niedermayer <mich...@niedermayer.cc> wrote: > > > > On Mon, Aug 31, 2020 at 11:14:07AM +0200, Paul B Mahol wrote: > > > >> On 8/31/20, Michael Niedermayer <mich...@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 <mich...@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
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".
