| Message ID | 20201220211524.5581-3-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avcodec/mjpegdec: Cleanup ff_smvjpeg_decoder() |
| Related | show |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
Unacceptable, please share privately sample that allows to reproduce this. On Sun, Dec 20, 2020 at 10:16 PM 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. > > Fixes: segfault > Fixes: OOM > Fixes: null pointer dereference > Fixes: left shift of negative value -12 > Fixes: out of array write > Fixes: > 25367/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-4865603750592512 > Fixes: > 25958/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-4851579923202048 > Fixes: > 25988/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-5643617157513216 > Fixes: > 25995/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-5177442380283904 > Fixes: > 25996/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-5663296026574848 > Fixes: > 26082/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-5126180416782336 > Fixes: > 27872/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-4916296355151872 > Fixes: > 28305/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-6041755829010432 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by > <https://github.com/google/oss-fuzz/tree/master/projects/ffmpegSigned-off-by>: > Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/cfhd.c | 154 ++++++++++++++++++++++++++++++++++++++++++++++ > libavcodec/cfhd.h | 4 ++ > 2 files changed, 158 insertions(+) > > diff --git a/libavcodec/cfhd.c b/libavcodec/cfhd.c > index 997beac7f4..5120e95f04 100644 > --- a/libavcodec/cfhd.c > +++ b/libavcodec/cfhd.c > @@ -363,6 +363,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) > { > @@ -376,6 +521,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); > > @@ -387,6 +534,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; > > -- > 2.17.1 > > _______________________________________________ > 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 Sun, Dec 20, 2020 at 10:18:40PM +0100, Paul B Mahol wrote:
> Unacceptable, please share privately sample that allows to reproduce this.
shared the ones which reproduce.
Please explain why this patch is unacceptable to you.
the CFHD decoder decodes header elements in the order in which they are
stored. The problem is that many have interdependancies yet there are
no checks for these. And where there are checks theres no protection
against changing dependancies after they have been used.
Basically CFHD allows an attacker to do absolutely anything
To pick a random example:
the code reading the SubbandNumber adjusts the level and then
checks its range based on transform_type. Yet transform_type
may be not set yet or may be subsequently changed.
That is issue 27872
One surely can try to add specific checks for all this but i doubt that will
result in secure code anytime soon. Its IMO better to fundamentally
fix this and not allow anything to occur in any multiplicity and order.
My posted patch is one way of many possible alternatives to move in that
direction
Thanks
[...]
On Tue, Dec 22, 2020 at 10:27 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Sun, Dec 20, 2020 at 10:18:40PM +0100, Paul B Mahol wrote: > > Unacceptable, please share privately sample that allows to reproduce > this. > > shared the ones which reproduce. > > Please explain why this patch is unacceptable to you. > > the CFHD decoder decodes header elements in the order in which they are > stored. The problem is that many have interdependancies yet there are > no checks for these. And where there are checks theres no protection > against changing dependancies after they have been used. > Basically CFHD allows an attacker to do absolutely anything > > To pick a random example: > the code reading the SubbandNumber adjusts the level and then > checks its range based on transform_type. Yet transform_type > may be not set yet or may be subsequently changed. > That is issue 27872 > > One surely can try to add specific checks for all this but i doubt that > will > result in secure code anytime soon. Its IMO better to fundamentally > fix this and not allow anything to occur in any multiplicity and order. > My posted patch is one way of many possible alternatives to move in that > direction > > Well, you forgot that when you check for order of tags, you basically still allow crash to happen, just this time crashing code path needs to follow correct parsing order. > Thanks > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > There will always be a question for which you do not know the correct > answer. > _______________________________________________ > 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 Wed, Dec 23, 2020 at 10:52:03AM +0100, Paul B Mahol wrote: > On Tue, Dec 22, 2020 at 10:27 PM Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > On Sun, Dec 20, 2020 at 10:18:40PM +0100, Paul B Mahol wrote: > > > Unacceptable, please share privately sample that allows to reproduce > > this. > > > > shared the ones which reproduce. > > > > Please explain why this patch is unacceptable to you. > > > > the CFHD decoder decodes header elements in the order in which they are > > stored. The problem is that many have interdependancies yet there are > > no checks for these. And where there are checks theres no protection > > against changing dependancies after they have been used. > > Basically CFHD allows an attacker to do absolutely anything > > > > To pick a random example: > > the code reading the SubbandNumber adjusts the level and then > > checks its range based on transform_type. Yet transform_type > > may be not set yet or may be subsequently changed. > > That is issue 27872 > > > > One surely can try to add specific checks for all this but i doubt that > > will > > result in secure code anytime soon. Its IMO better to fundamentally > > fix this and not allow anything to occur in any multiplicity and order. > > My posted patch is one way of many possible alternatives to move in that > > direction > > > > > Well, you forgot that when you check for order of tags, you basically still > allow > crash to happen, just this time crashing code path needs to follow correct > parsing order. I dont see how that would be possible with the correct order of tags transform_type is set and checked and can after that only be 0 or 2 the crash required it to be -1. As transform_type is marked as a mandatory element in the table and -1 is not a possible value after it i dont see how that could work. But maybe iam missing something thx [...]
On Wed, Dec 23, 2020 at 3:59 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Dec 23, 2020 at 10:52:03AM +0100, Paul B Mahol wrote: > > On Tue, Dec 22, 2020 at 10:27 PM Michael Niedermayer > <michael@niedermayer.cc> > > wrote: > > > > > On Sun, Dec 20, 2020 at 10:18:40PM +0100, Paul B Mahol wrote: > > > > Unacceptable, please share privately sample that allows to reproduce > > > this. > > > > > > shared the ones which reproduce. > > > > > > Please explain why this patch is unacceptable to you. > > > > > > the CFHD decoder decodes header elements in the order in which they are > > > stored. The problem is that many have interdependancies yet there are > > > no checks for these. And where there are checks theres no protection > > > against changing dependancies after they have been used. > > > Basically CFHD allows an attacker to do absolutely anything > > > > > > To pick a random example: > > > the code reading the SubbandNumber adjusts the level and then > > > checks its range based on transform_type. Yet transform_type > > > may be not set yet or may be subsequently changed. > > > That is issue 27872 > > > > > > One surely can try to add specific checks for all this but i doubt that > > > will > > > result in secure code anytime soon. Its IMO better to fundamentally > > > fix this and not allow anything to occur in any multiplicity and order. > > > My posted patch is one way of many possible alternatives to move in > that > > > direction > > > > > > > > Well, you forgot that when you check for order of tags, you basically > still > > allow > > crash to happen, just this time crashing code path needs to follow > correct > > parsing order. > > I dont see how that would be possible > > with the correct order of tags > transform_type is set and checked and can after that only be 0 or 2 > the crash required it to be -1. As transform_type is marked as a mandatory > element in the table and -1 is not a possible value after it i dont see > how that could work. > But maybe iam missing something > Transform type is always set, so that one can always be checked that it is set to correct value. But above code is too complex for my alternative fix.
diff --git a/libavcodec/cfhd.c b/libavcodec/cfhd.c index 997beac7f4..5120e95f04 100644 --- a/libavcodec/cfhd.c +++ b/libavcodec/cfhd.c @@ -363,6 +363,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) { @@ -376,6 +521,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); @@ -387,6 +534,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. Fixes: segfault Fixes: OOM Fixes: null pointer dereference Fixes: left shift of negative value -12 Fixes: out of array write Fixes: 25367/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-4865603750592512 Fixes: 25958/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-4851579923202048 Fixes: 25988/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-5643617157513216 Fixes: 25995/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-5177442380283904 Fixes: 25996/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-5663296026574848 Fixes: 26082/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-5126180416782336 Fixes: 27872/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-4916296355151872 Fixes: 28305/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-6041755829010432 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/cfhd.c | 154 ++++++++++++++++++++++++++++++++++++++++++++++ libavcodec/cfhd.h | 4 ++ 2 files changed, 158 insertions(+)