| Message ID | 20200830220658.25369-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v4] avcodec/cfhd: More strictly check tag order and multiplicity | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On 8/31/20, Michael Niedermayer <michael@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@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.
On Mon, Aug 31, 2020 at 11:14:07AM +0200, Paul B Mahol wrote: > On 8/31/20, Michael Niedermayer <michael@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@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. Thanks
On 8/31/20, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Mon, Aug 31, 2020 at 11:14:07AM +0200, Paul B Mahol wrote: >> On 8/31/20, Michael Niedermayer <michael@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@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.
On Mon, Aug 31, 2020 at 07:42:20PM +0200, Paul B Mahol wrote: > On 8/31/20, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Mon, Aug 31, 2020 at 11:14:07AM +0200, Paul B Mahol wrote: > >> On 8/31/20, Michael Niedermayer <michael@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@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 [...]
On Sat, Sep 19, 2020 at 7:45 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Mon, Aug 31, 2020 at 07:42:20PM +0200, Paul B Mahol wrote: > > On 8/31/20, Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Mon, Aug 31, 2020 at 11:14:07AM +0200, Paul B Mahol wrote: > > >> On 8/31/20, Michael Niedermayer <michael@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@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
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@niedermayer.cc> > wrote: > > > On Mon, Aug 31, 2020 at 07:42:20PM +0200, Paul B Mahol wrote: > > > On 8/31/20, Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > On Mon, Aug 31, 2020 at 11:14:07AM +0200, Paul B Mahol wrote: > > > >> On 8/31/20, Michael Niedermayer <michael@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@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" [...]
diff --git a/libavcodec/cfhd.c b/libavcodec/cfhd.c index ea35f03869..6caaeb9c19 100644 --- a/libavcodec/cfhd.c +++ b/libavcodec/cfhd.c @@ -361,6 +361,151 @@ static int alloc_buffers(AVCodecContext *avctx) return 0; } +typedef struct TagDescriptor { + int16_t previous_marker1; + int16_t previous_marker2; + uint8_t mandatory : 1; + uint8_t single : 1; +} TagDescriptor; + +static TagDescriptor tag_descriptor[LastTag]={ + [SampleType /* 1*/] = { .previous_marker1 = 0x0c0c, .previous_marker2 = -1, }, + [SampleIndexTable /* 2*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 1 }, + [ 3 ] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [BitstreamMarker /* 4*/] = { }, + [VersionMajor /* 5*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [VersionMinor /* 6*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [VersionRevision /* 7*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [VersionEdit /* 8*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [ 9 ] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [TransformType /* 10*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 1 }, + [NumFrames /* 11*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 1 }, + [ChannelCount /* 12*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 1 }, + [WaveletCount /* 13*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 1 }, + [SubbandCount /* 14*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 1 }, + [NumSpatial /* 15*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 1 }, + [FirstWavelet /* 16*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 1 }, + [ 17 ] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [GroupTrailer /* 18*/] = { .previous_marker1 = 0x0c0c, .single = 1, .mandatory = 0 }, + [FrameType /* 19*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [ImageWidth /* 20*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [ImageHeight /* 21*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [ 22 ] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [FrameIndex /* 23*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [ 24 ] = { .previous_marker1 = 0x0c0c, .single = 1, .mandatory = 0 }, + [LowpassSubband /* 25*/] = { .previous_marker1 = 0x1a4a, .single = 1, .mandatory = 1 }, + [NumLevels /* 26*/] = { .previous_marker1 = 0x1a4a, .single = 1, .mandatory = 1 }, + [LowpassWidth /* 27*/] = { .previous_marker1 = 0x1a4a, .single = 1, .mandatory = 1 }, + [LowpassHeight /* 28*/] = { .previous_marker1 = 0x1a4a, .single = 1, .mandatory = 1 }, + [ 29 ] = { .previous_marker1 = 0x1a4a, .single = 1, .mandatory = 1 }, + [ 30 ] = { .previous_marker1 = 0x1a4a, .single = 1, .mandatory = 1 }, + [ 31 ] = { .previous_marker1 = 0x1a4a, .single = 1, .mandatory = 1 }, + [ 32 ] = { .previous_marker1 = 0x1a4a, .single = 1, .mandatory = 1 }, + [PixelOffset /* 33*/] = { .previous_marker1 = 0x1a4a, .single = 1, .mandatory = 1 }, + [LowpassQuantization/*34*/] = { .previous_marker1 = 0x1a4a, .single = 1, .mandatory = 1 }, + [LowpassPrecision /* 35*/] = { .previous_marker1 = 0x1a4a, .single = 1, .mandatory = 1 }, + [ 36 ] = { .previous_marker1 = 0x1a4a, .single = 1, .mandatory = 0 }, + [WaveletType /* 37*/] = { .previous_marker1 = 0x0d0d, .single = 1, .mandatory = 1 }, + [WaveletNumber /* 38*/] = { .previous_marker1 = 0x0d0d, .single = 1, .mandatory = 1 }, + [WaveletLevel /* 39*/] = { .previous_marker1 = 0x0d0d, .single = 1, .mandatory = 1 }, + [NumBands /* 40*/] = { .previous_marker1 = 0x0d0d, .single = 1, .mandatory = 1 }, + [HighpassWidth /* 41*/] = { .previous_marker1 = 0x0d0d, .single = 1, .mandatory = 1 }, + [HighpassHeight /* 42*/] = { .previous_marker1 = 0x0d0d, .single = 1, .mandatory = 1 }, + [LowpassBorder /* 43*/] = { .previous_marker1 = 0x0d0d, .single = 1, .mandatory = 1 }, + [HighpassBorder /* 44*/] = { .previous_marker1 = 0x0d0d, .single = 1, .mandatory = 1 }, + [LowpassScale /* 45*/] = { .previous_marker1 = 0x0d0d, .single = 1, .mandatory = 1 }, + [LowpassDivisor /* 46*/] = { .previous_marker1 = 0x0d0d, .single = 1, .mandatory = 1 }, + [ 47 ] = { .previous_marker1 = 0x0d0d, .single = 1, .mandatory = 0 }, + [SubbandNumber /* 48*/] = { .previous_marker1 = 0x0e0e, .single = 1, .mandatory = 1 }, + [BandWidth /* 49*/] = { .previous_marker1 = 0x0e0e, .single = 1, .mandatory = 1 }, + [BandHeight /* 50*/] = { .previous_marker1 = 0x0e0e, .single = 1, .mandatory = 1 }, + [SubbandBand /* 51*/] = { .previous_marker1 = 0x0e0e, .single = 1, .mandatory = 1 }, + [BandEncoding /* 52*/] = { .previous_marker1 = 0x0e0e, .single = 1, .mandatory = 1 }, + [Quantization /* 53*/] = { .previous_marker1 = 0x0e0e, .single = 1, .mandatory = 1 }, + [BandScale /* 54*/] = { .previous_marker1 = 0x0e0e, .single = 1, .mandatory = 1 }, + [BandHeader /* 55*/] = { .previous_marker1 = 0x0e0e, .single = 1, .mandatory = 1 }, + [BandTrailer /* 56*/] = { .previous_marker1 = 0x0e0e, .single = 1, .mandatory = 1 }, + [ChannelNumber /* 62*/] = { .previous_marker1 = 0x0c0c, .single = 1, .mandatory = 0 }, + [ 63 ] = { .previous_marker1 = -1, .single = 1, .mandatory = 1 }, + [ 64 ] = { .previous_marker1 = -1, .single = 1, .mandatory = 1 }, + [ 65 ] = { .previous_marker1 = -1, .single = 1, .mandatory = 1 }, + [ 66 ] = { .previous_marker1 = -1, .single = 1, .mandatory = 1 }, + [SampleFlags /* 68*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [FrameNumber /* 69*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 1 }, + [Precision /* 70*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [InputFormat /* 71*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 1 }, + [BandCodingFlags /* 72*/] = { .previous_marker1 = 0x0e0e, .single = 1, .mandatory = 0 }, + [ 73 ] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [PeakLevel /* 74*/] = { .previous_marker1 = 0x0e0e, .single = 1, .mandatory = 0 }, + [PeakOffsetLow /* 75*/] = { .previous_marker1 = 0x0e0e, .single = 1, .mandatory = 0 }, + [PeakOffsetHigh /* 76*/] = { .previous_marker1 = 0x0e0e, .single = 1, .mandatory = 0 }, + [Version /* 79*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [ 80 ] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [ 81 ] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [BandSecondPass /* 82*/] = { }, + [PrescaleTable /* 83*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [EncodedFormat /* 84*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [DisplayHeight /* 85*/] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [ 91 ] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [ 92 ] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [ 93 ] = { .previous_marker1 = -1, .single = 1, .mandatory = 0 }, + [ChannelWidth /* 104*/] = { }, + [ChannelHeight /* 105*/] = { }, +}; + +static int handle_tag_order(CFHDContext *s, int tag, int data) +{ + TagDescriptor *desc; + int atag = abs(tag); + int i; + + // We do not restrict tags outside the enum + if (atag >= LastTag) + return 0; + + desc= &tag_descriptor[atag]; + if (desc->single && s->tag_count[atag]) + return AVERROR_INVALIDDATA; + + if (desc->previous_marker1 && s->previous_marker != desc->previous_marker1) { + if (!desc->previous_marker2 || s->previous_marker != desc->previous_marker2) + return AVERROR_INVALIDDATA; + } else if (tag == BitstreamMarker) { + if (s->previous_marker == -1 && data == 0x1a4a) { + ; + } else if (s->previous_marker == 0x1a4a && data == 0x0f0f) { + ; + } else if (s->previous_marker == 0x0f0f && data == 0x1b4b) { + ; + } else if (s->previous_marker == 0x1b4b && data == 0x0d0d) { + ; + } else if (s->previous_marker == 0x0d0d && data == 0x0e0e) { + ; + } else if (s->previous_marker == 0x0e0e && (data == 0x0c0c || data == 0x0e0e)) { + ; + } else if (s->previous_marker == 0x0c0c && (data == 0x0d0d || data == 0x1a4a)) { + ; + } else + return AVERROR_INVALIDDATA; + + for (i = 0; i<LastTag; i++) { + // Whenever we switch to a new marker we check the mandatory elements of the previous + if (tag_descriptor[i].previous_marker1 == s->previous_marker && tag_descriptor[i].mandatory && !s->tag_count[i]) { + return AVERROR_INVALIDDATA; + } + + if (tag_descriptor[i].previous_marker1 == data) + s->tag_count[i] = 0; + } + s->previous_marker = data; + } else if (!desc->previous_marker1) + return AVERROR_INVALIDDATA; + + s->tag_count[atag]++; + + return 0; +} + static int cfhd_decode(AVCodecContext *avctx, void *data, int *got_frame, AVPacket *avpkt) { @@ -374,6 +519,8 @@ static int cfhd_decode(AVCodecContext *avctx, void *data, int *got_frame, init_frame_defaults(s); s->planes = av_pix_fmt_count_planes(s->coded_format); + s->previous_marker = -1; + memset(s->tag_count, 0, sizeof(s->tag_count)); bytestream2_init(&gb, avpkt->data, avpkt->size); @@ -385,6 +532,13 @@ static int cfhd_decode(AVCodecContext *avctx, void *data, int *got_frame, uint16_t abstag = abs(tag); int8_t abs_tag8 = abs(tag8); uint16_t data = bytestream2_get_be16(&gb); + + ret = handle_tag_order(s, tag, data); + if (ret < 0) { + av_log(avctx, AV_LOG_DEBUG, "Unexpected TAG %d data %X in %X\n", tag, data, s->previous_marker); + return ret; + } + if (abs_tag8 >= 0x60 && abs_tag8 <= 0x6f) { av_log(avctx, AV_LOG_DEBUG, "large len %x\n", ((tagu & 0xff) << 16) | data); } else if (tag == SampleFlags) { diff --git a/libavcodec/cfhd.h b/libavcodec/cfhd.h index 8ea91270cd..802c338f13 100644 --- a/libavcodec/cfhd.h +++ b/libavcodec/cfhd.h @@ -93,6 +93,7 @@ enum CFHDParam { DisplayHeight = 85, ChannelWidth = 104, ChannelHeight = 105, + LastTag, }; #define VLC_BITS 9 @@ -184,6 +185,9 @@ typedef struct CFHDContext { Plane plane[4]; Peak peak; + int previous_marker; + int tag_count[LastTag]; + CFHDDSPContext dsp; } CFHDContext;
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@niedermayer.cc> --- libavcodec/cfhd.c | 154 ++++++++++++++++++++++++++++++++++++++++++++++ libavcodec/cfhd.h | 4 ++ 2 files changed, 158 insertions(+)