diff mbox series

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

Message ID 20201220211524.5581-3-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/3] avcodec/mjpegdec: Cleanup ff_smvjpeg_decoder() | expand

Checks

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

Commit Message

Michael Niedermayer Dec. 20, 2020, 9:15 p.m. UTC 
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(+)

Comments

Paul B Mahol Dec. 20, 2020, 9:18 p.m. UTC | #1 
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".
Michael Niedermayer Dec. 22, 2020, 9:27 p.m. UTC | #2 
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

[...]
Paul B Mahol Dec. 23, 2020, 9:52 a.m. UTC | #3 
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".
Michael Niedermayer Dec. 23, 2020, 2:59 p.m. UTC | #4 
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

[...]
Paul B Mahol Dec. 23, 2020, 3:02 p.m. UTC | #5 
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.
Michael Niedermayer March 30, 2021, 4:49 p.m. UTC | #6 
On Sun, Dec 20, 2020 at 10:15:24PM +0100, Michael Niedermayer 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

The following issues have been found by the fuzzer in CFHD since this was posted
With this applied none is reproducable


29754/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-6333598414274560
==18805==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000d6875d bp 0x000000000000 sp 0x7ffde47353d8 T0)
==18805==The signal is caused by a READ memory access.
==18805==Hint: address points to the zero page.
    #0 0xd6875c in ff_cfhd_vert_filter_sse2 libavcodec/x86/cfhddsp.asm:383



30519/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-6298424511168512
libavcodec/cfhddsp.c:36:41: runtime error: load of null pointer of type 'const int16_t' (aka 'const short')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/cfhddsp.c:36:41 in
AddressSanitizer:DEADLYSIGNAL
=================================================================
==18874==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000005450d5 bp 0x7ffc0a6c6ee0 sp 0x7ffc0a6c6d60 T0)
==18874==The signal is caused by a READ memory access.
==18874==Hint: address points to the zero page.
    #0 0x5450d4 in filter libavcodec/cfhddsp.c:36:41
    #1 0x5450d4 in vert_filter libavcodec/cfhddsp.c:74
    #2 0x528cea in cfhd_decode libavcodec/cfhd.c:1167:13
    #3 0x57d746 in decode_simple_internal libavcodec/decode.c:327:15
    #4 0x557ec7 in decode_simple_receive_frame libavcodec/decode.c:526:15
    #5 0x557ec7 in decode_receive_frame_internal libavcodec/decode.c:546
    #6 0x5574ea in avcodec_send_packet libavcodec/decode.c:608:15



30739/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-5011292836462592
==18879==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f2aa32a684e at pc 0x00000053ee8f bp 0x7ffca5380fe0 sp 0x7ffca5380fd8
WRITE of size 2 at 0x7f2aa32a684e thread T0
    #0 0x53ee8e in interlaced_vertical_filter libavcodec/cfhd.c:204:30
    #1 0x52c088 in cfhd_decode libavcodec/cfhd.c:1273:21
    #2 0x57d746 in decode_simple_internal libavcodec/decode.c:327:15
    #3 0x557ec7 in decode_simple_receive_frame libavcodec/decode.c:526:15
    #4 0x557ec7 in decode_receive_frame_internal libavcodec/decode.c:546
    #5 0x5574ea in avcodec_send_packet libavcodec/decode.c:608:15


    
32124/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-5425980681355264
==18753==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f3a5006284e at pc 0x00000054c97e bp 0x7ffc0327a620 sp 0x7ffc0327a618
WRITE of size 2 at 0x7f3a5006284e thread T0
    #0 0x54c97d in filter libavcodec/cfhddsp.c:52:36
    #1 0x54c97d in horiz_filter_clip libavcodec/cfhddsp.c:97
    #2 0x52cbed in cfhd_decode libavcodec/cfhd.c:1239:21
    #3 0x57d746 in decode_simple_internal libavcodec/decode.c:327:15
    #4 0x557ec7 in decode_simple_receive_frame libavcodec/decode.c:526:15
    #5 0x557ec7 in decode_receive_frame_internal libavcodec/decode.c:546
    #6 0x5574ea in avcodec_send_packet libavcodec/decode.c:608:15


[...]
Paul B Mahol March 30, 2021, 5:50 p.m. UTC | #7 
Please share files privately, do not apply non fix for this issue.

Give up with such this non-solution.

On Tue, Mar 30, 2021 at 6:49 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Sun, Dec 20, 2020 at 10:15:24PM +0100, Michael Niedermayer 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
>
> The following issues have been found by the fuzzer in CFHD since this was
> posted
> With this applied none is reproducable
>
>
>
> 29754/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-6333598414274560
> ==18805==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000
> (pc 0x000000d6875d bp 0x000000000000 sp 0x7ffde47353d8 T0)
> ==18805==The signal is caused by a READ memory access.
> ==18805==Hint: address points to the zero page.
>     #0 0xd6875c in ff_cfhd_vert_filter_sse2 libavcodec/x86/cfhddsp.asm:383
>
>
>
>
> 30519/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-6298424511168512
> libavcodec/cfhddsp.c:36:41: runtime error: load of null pointer of type
> 'const int16_t' (aka 'const short')
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
> libavcodec/cfhddsp.c:36:41 in
> AddressSanitizer:DEADLYSIGNAL
> =================================================================
> ==18874==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000
> (pc 0x0000005450d5 bp 0x7ffc0a6c6ee0 sp 0x7ffc0a6c6d60 T0)
> ==18874==The signal is caused by a READ memory access.
> ==18874==Hint: address points to the zero page.
>     #0 0x5450d4 in filter libavcodec/cfhddsp.c:36:41
>     #1 0x5450d4 in vert_filter libavcodec/cfhddsp.c:74
>     #2 0x528cea in cfhd_decode libavcodec/cfhd.c:1167:13
>     #3 0x57d746 in decode_simple_internal libavcodec/decode.c:327:15
>     #4 0x557ec7 in decode_simple_receive_frame libavcodec/decode.c:526:15
>     #5 0x557ec7 in decode_receive_frame_internal libavcodec/decode.c:546
>     #6 0x5574ea in avcodec_send_packet libavcodec/decode.c:608:15
>
>
>
>
> 30739/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-5011292836462592
> ==18879==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x7f2aa32a684e at pc 0x00000053ee8f bp 0x7ffca5380fe0 sp 0x7ffca5380fd8
> WRITE of size 2 at 0x7f2aa32a684e thread T0
>     #0 0x53ee8e in interlaced_vertical_filter libavcodec/cfhd.c:204:30
>     #1 0x52c088 in cfhd_decode libavcodec/cfhd.c:1273:21
>     #2 0x57d746 in decode_simple_internal libavcodec/decode.c:327:15
>     #3 0x557ec7 in decode_simple_receive_frame libavcodec/decode.c:526:15
>     #4 0x557ec7 in decode_receive_frame_internal libavcodec/decode.c:546
>     #5 0x5574ea in avcodec_send_packet libavcodec/decode.c:608:15
>
>
>
>
> 32124/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-5425980681355264
> ==18753==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x7f3a5006284e at pc 0x00000054c97e bp 0x7ffc0327a620 sp 0x7ffc0327a618
> WRITE of size 2 at 0x7f3a5006284e thread T0
>     #0 0x54c97d in filter libavcodec/cfhddsp.c:52:36
>     #1 0x54c97d in horiz_filter_clip libavcodec/cfhddsp.c:97
>     #2 0x52cbed in cfhd_decode libavcodec/cfhd.c:1239:21
>     #3 0x57d746 in decode_simple_internal libavcodec/decode.c:327:15
>     #4 0x557ec7 in decode_simple_receive_frame libavcodec/decode.c:526:15
>     #5 0x557ec7 in decode_receive_frame_internal libavcodec/decode.c:546
>     #6 0x5574ea in avcodec_send_packet libavcodec/decode.c:608:15
>
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> If you think the mosad wants you dead since a long time then you are either
> wrong or dead since a long time.
> _______________________________________________
> 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".
Paul B Mahol March 31, 2021, 6:26 p.m. UTC | #8 
I can not reproduce any crash either with address sanitizer or valgrind
with samples you provided.
Are you sure this apply to master?

On Tue, Mar 30, 2021 at 7:50 PM Paul B Mahol <onemda@gmail.com> wrote:

> Please share files privately, do not apply non fix for this issue.
>
> Give up with such this non-solution.
>
> On Tue, Mar 30, 2021 at 6:49 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
>
>> On Sun, Dec 20, 2020 at 10:15:24PM +0100, Michael Niedermayer 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
>>
>> The following issues have been found by the fuzzer in CFHD since this was
>> posted
>> With this applied none is reproducable
>>
>>
>>
>> 29754/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-6333598414274560
>> ==18805==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000
>> (pc 0x000000d6875d bp 0x000000000000 sp 0x7ffde47353d8 T0)
>> ==18805==The signal is caused by a READ memory access.
>> ==18805==Hint: address points to the zero page.
>>     #0 0xd6875c in ff_cfhd_vert_filter_sse2 libavcodec/x86/cfhddsp.asm:383
>>
>>
>>
>>
>> 30519/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-6298424511168512
>> libavcodec/cfhddsp.c:36:41: runtime error: load of null pointer of type
>> 'const int16_t' (aka 'const short')
>> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
>> libavcodec/cfhddsp.c:36:41 in
>> AddressSanitizer:DEADLYSIGNAL
>> =================================================================
>> ==18874==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000
>> (pc 0x0000005450d5 bp 0x7ffc0a6c6ee0 sp 0x7ffc0a6c6d60 T0)
>> ==18874==The signal is caused by a READ memory access.
>> ==18874==Hint: address points to the zero page.
>>     #0 0x5450d4 in filter libavcodec/cfhddsp.c:36:41
>>     #1 0x5450d4 in vert_filter libavcodec/cfhddsp.c:74
>>     #2 0x528cea in cfhd_decode libavcodec/cfhd.c:1167:13
>>     #3 0x57d746 in decode_simple_internal libavcodec/decode.c:327:15
>>     #4 0x557ec7 in decode_simple_receive_frame libavcodec/decode.c:526:15
>>     #5 0x557ec7 in decode_receive_frame_internal libavcodec/decode.c:546
>>     #6 0x5574ea in avcodec_send_packet libavcodec/decode.c:608:15
>>
>>
>>
>>
>> 30739/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-5011292836462592
>> ==18879==ERROR: AddressSanitizer: heap-buffer-overflow on address
>> 0x7f2aa32a684e at pc 0x00000053ee8f bp 0x7ffca5380fe0 sp 0x7ffca5380fd8
>> WRITE of size 2 at 0x7f2aa32a684e thread T0
>>     #0 0x53ee8e in interlaced_vertical_filter libavcodec/cfhd.c:204:30
>>     #1 0x52c088 in cfhd_decode libavcodec/cfhd.c:1273:21
>>     #2 0x57d746 in decode_simple_internal libavcodec/decode.c:327:15
>>     #3 0x557ec7 in decode_simple_receive_frame libavcodec/decode.c:526:15
>>     #4 0x557ec7 in decode_receive_frame_internal libavcodec/decode.c:546
>>     #5 0x5574ea in avcodec_send_packet libavcodec/decode.c:608:15
>>
>>
>>
>>
>> 32124/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_CFHD_fuzzer-5425980681355264
>> ==18753==ERROR: AddressSanitizer: heap-buffer-overflow on address
>> 0x7f3a5006284e at pc 0x00000054c97e bp 0x7ffc0327a620 sp 0x7ffc0327a618
>> WRITE of size 2 at 0x7f3a5006284e thread T0
>>     #0 0x54c97d in filter libavcodec/cfhddsp.c:52:36
>>     #1 0x54c97d in horiz_filter_clip libavcodec/cfhddsp.c:97
>>     #2 0x52cbed in cfhd_decode libavcodec/cfhd.c:1239:21
>>     #3 0x57d746 in decode_simple_internal libavcodec/decode.c:327:15
>>     #4 0x557ec7 in decode_simple_receive_frame libavcodec/decode.c:526:15
>>     #5 0x557ec7 in decode_receive_frame_internal libavcodec/decode.c:546
>>     #6 0x5574ea in avcodec_send_packet libavcodec/decode.c:608:15
>>
>>
>> [...]
>> --
>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> If you think the mosad wants you dead since a long time then you are
>> either
>> wrong or dead since a long time.
>> _______________________________________________
>> 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".
>
>
Paul B Mahol April 1, 2021, 7:22 p.m. UTC | #9 
Try this attached patch. I have not looked at all samples, as some allocate
too much memory for my system.
But this patch points where real bugs are, unlike yours patch which hides
real bugs even more.
Michael Niedermayer April 1, 2021, 10:25 p.m. UTC | #10 
On Thu, Apr 01, 2021 at 09:22:23PM +0200, Paul B Mahol wrote:
> Try this attached patch. I have not looked at all samples, as some allocate
> too much memory for my system.

> But this patch points where real bugs are, unlike yours patch which hides
> real bugs even more.

I would appreciate if cfhd wouldnt have so many real bugs.
Your approach seems to be to fix what the fuzzer finds. What my patch was
moving toward is to make the code more secure and robust not to fix individual
bugs. My patch was never intended to be the end of such improvment, but with
the first stage being rejected iam of course not putting time in the next ...

but thats not so importrant now, whats important is the bugs here
and your patch eliminates all of the current group but one. Thats good!
Heres what remains:
ffmpeg -threads 1 -i dec_fuzzer-30739.nut -f null -

[cfhd @ 0x16b0c6c0] Sample format of 1039 is not implemented. Update your FFmpeg version to the newest one from Git. If the problem still occurs, it means that your file has a feature which has not been implemented.
Error while decoding stream #0:0: Not yet implemented in FFmpeg, patches welcome
==17282==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
==17282==    by 0x1233DEB: av_log_default_callback (log.c:397)
==17282==    by 0x1234092: av_vlog (log.c:432)
==17282==    by 0x1233EF1: av_log (log.c:411)
==17282==    by 0x254285: process_input_packet (ffmpeg.c:2718)
==17282==    by 0x25BB79: process_input (ffmpeg.c:4606)
==17282==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==17282==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==17282==    by 0x25CB3F: main (ffmpeg.c:5005)
==17282== Conditional jump or move depends on uninitialised value(s)
==17282==    at 0x82BBF4: av_clip_uintp2_c (common.h:304)
==17282==    by 0x82C915: interlaced_vertical_filter (cfhd.c:205)
==17282==    by 0x83424E: cfhd_decode (cfhd.c:1278)
==17282==    by 0x860064: decode_simple_internal (decode.c:327)
==17282==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
==17282==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
==17282==    by 0x861019: avcodec_send_packet (decode.c:608)
==17282==    by 0x2525A7: decode (ffmpeg.c:2285)
==17282==    by 0x252DC7: decode_video (ffmpeg.c:2425)
==17282==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
==17282==    by 0x25BB79: process_input (ffmpeg.c:4606)
==17282==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==17282==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==17282==    by 0x25CB3F: main (ffmpeg.c:5005)
==17282==  Uninitialised value was created by a heap allocation
==17282==    at 0x4C33E76: memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==17282==    by 0x4C33F91: posix_memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==17282==    by 0x1236BA6: av_malloc (mem.c:86)
==17282==    by 0x1236DB8: av_malloc_array (mem.c:187)
==17282==    by 0x82D072: alloc_buffers (cfhd.c:296)
==17282==    by 0x82F8DD: cfhd_decode (cfhd.c:664)
==17282==    by 0x860064: decode_simple_internal (decode.c:327)
==17282==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
==17282==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
==17282==    by 0x861019: avcodec_send_packet (decode.c:608)
==17282==    by 0x2525A7: decode (ffmpeg.c:2285)
==17282==    by 0x252DC7: decode_video (ffmpeg.c:2425)
==17282==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
==17282==    by 0x25BB79: process_input (ffmpeg.c:4606)
==17282==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==17282==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==17282==    by 0x25CB3F: main (ffmpeg.c:5005)
==17282== 
==17282== Conditional jump or move depends on uninitialised value(s)
==17282==    at 0x82BBF4: av_clip_uintp2_c (common.h:304)
==17282==    by 0x82C93C: interlaced_vertical_filter (cfhd.c:206)
==17282==    by 0x83424E: cfhd_decode (cfhd.c:1278)
==17282==    by 0x860064: decode_simple_internal (decode.c:327)
==17282==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
==17282==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
==17282==    by 0x861019: avcodec_send_packet (decode.c:608)
==17282==    by 0x2525A7: decode (ffmpeg.c:2285)
==17282==    by 0x252DC7: decode_video (ffmpeg.c:2425)
==17282==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
==17282==    by 0x25BB79: process_input (ffmpeg.c:4606)
==17282==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==17282==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==17282==    by 0x25CB3F: main (ffmpeg.c:5005)
==17282==  Uninitialised value was created by a heap allocation
==17282==    at 0x4C33E76: memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==17282==    by 0x4C33F91: posix_memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==17282==    by 0x1236BA6: av_malloc (mem.c:86)
==17282==    by 0x1236DB8: av_malloc_array (mem.c:187)
==17282==    by 0x82D072: alloc_buffers (cfhd.c:296)
==17282==    by 0x82F8DD: cfhd_decode (cfhd.c:664)
==17282==    by 0x860064: decode_simple_internal (decode.c:327)
==17282==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
==17282==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
==17282==    by 0x861019: avcodec_send_packet (decode.c:608)
==17282==    by 0x2525A7: decode (ffmpeg.c:2285)
==17282==    by 0x252DC7: decode_video (ffmpeg.c:2425)
==17282==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
==17282==    by 0x25BB79: process_input (ffmpeg.c:4606)
==17282==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==17282==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==17282==    by 0x25CB3F: main (ffmpeg.c:5005)
==17282== 
==17282== Invalid write of size 2
==17282==    at 0x82C956: interlaced_vertical_filter (cfhd.c:206)
==17282==    by 0x83424E: cfhd_decode (cfhd.c:1278)
==17282==    by 0x860064: decode_simple_internal (decode.c:327)
==17282==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
==17282==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
==17282==    by 0x861019: avcodec_send_packet (decode.c:608)
==17282==    by 0x2525A7: decode (ffmpeg.c:2285)
==17282==    by 0x252DC7: decode_video (ffmpeg.c:2425)
==17282==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
==17282==    by 0x25BB79: process_input (ffmpeg.c:4606)
==17282==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==17282==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==17282==    by 0x25CB3F: main (ffmpeg.c:5005)
==17282==  Address 0x2a0cd24e is 2,621,518 bytes inside a block of size 2,621,519 alloc'd
==17282==    at 0x4C33E76: memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==17282==    by 0x4C33F91: posix_memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==17282==    by 0x1236BA6: av_malloc (mem.c:86)
==17282==    by 0x121C724: av_buffer_alloc (buffer.c:72)
==17282==    by 0x121C79F: av_buffer_allocz (buffer.c:85)
==17282==    by 0x121D15C: pool_alloc_buffer (buffer.c:351)
==17282==    by 0x121D2A3: av_buffer_pool_get (buffer.c:388)
==17282==    by 0x863DAC: video_get_buffer (decode.c:1663)
==17282==    by 0x863F9B: avcodec_default_get_buffer2 (decode.c:1702)
==17282==    by 0x254E5A: get_buffer (ffmpeg.c:2943)
==17282==    by 0x864A5A: ff_get_buffer (decode.c:1937)
==17282==    by 0xB219CE: thread_get_buffer_internal (pthread_frame.c:1006)
==17282==    by 0xB21E2B: ff_thread_get_buffer (pthread_frame.c:1098)
==17282==    by 0x82F9FA: cfhd_decode (cfhd.c:682)
==17282==    by 0x860064: decode_simple_internal (decode.c:327)
==17282==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
==17282==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
==17282==    by 0x861019: avcodec_send_packet (decode.c:608)
==17282==    by 0x2525A7: decode (ffmpeg.c:2285)
==17282==    by 0x252DC7: decode_video (ffmpeg.c:2425)
==17282== 
[cfhd @ 0x16b0c6c0] Invalid plane dimensions
==17282==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
==17282==    by 0x1233DEB: av_log_default_callback (log.c:397)
==17282==    by 0x1234092: av_vlog (log.c:432)
==17282==    by 0x1233EF1: av_log (log.c:411)
==17282==    by 0x832813: cfhd_decode (cfhd.c:1096)
==17282==    by 0x860064: decode_simple_internal (decode.c:327)
==17282==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
==17282==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
==17282==    by 0x861019: avcodec_send_packet (decode.c:608)
==17282==    by 0x2525A7: decode (ffmpeg.c:2285)
==17282==    by 0x252DC7: decode_video (ffmpeg.c:2425)
==17282==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
==17282==    by 0x25BB79: process_input (ffmpeg.c:4606)
==17282==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==17282==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==17282==    by 0x25CB3F: main (ffmpeg.c:5005)
Error while decoding stream #0:0: Invalid argument
==17282==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
==17282==    by 0x1233DEB: av_log_default_callback (log.c:397)
==17282==    by 0x1234092: av_vlog (log.c:432)
==17282==    by 0x1233EF1: av_log (log.c:411)
==17282==    by 0x254285: process_input_packet (ffmpeg.c:2718)
==17282==    by 0x25BB79: process_input (ffmpeg.c:4606)
==17282==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==17282==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==17282==    by 0x25CB3F: main (ffmpeg.c:5005)
[cfhd @ 0x16b0c6c0] Invalid dimensions
==17282==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
==17282==    by 0x1233DEB: av_log_default_callback (log.c:397)
==17282==    by 0x1234092: av_vlog (log.c:432)
==17282==    by 0x1233EF1: av_log (log.c:411)
==17282==    by 0x830E9D: cfhd_decode (cfhd.c:897)
==17282==    by 0x860064: decode_simple_internal (decode.c:327)
==17282==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
==17282==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
==17282==    by 0x861019: avcodec_send_packet (decode.c:608)
==17282==    by 0x2525A7: decode (ffmpeg.c:2285)
==17282==    by 0x252DC7: decode_video (ffmpeg.c:2425)
==17282==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
==17282==    by 0x25BB79: process_input (ffmpeg.c:4606)
==17282==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==17282==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==17282==    by 0x25CB3F: main (ffmpeg.c:5005)
Error while decoding stream #0:0: Invalid argument
==17282==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
==17282==    by 0x1233DEB: av_log_default_callback (log.c:397)
==17282==    by 0x1234092: av_vlog (log.c:432)
==17282==    by 0x1233EF1: av_log (log.c:411)
==17282==    by 0x254285: process_input_packet (ffmpeg.c:2718)
==17282==    by 0x25BB79: process_input (ffmpeg.c:4606)
==17282==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==17282==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==17282==    by 0x25CB3F: main (ffmpeg.c:5005)
[cfhd @ 0x16b0c6c0] Invalid dimensionsme=00:00:00.00 bitrate=N/A speed=5.23e-06x    
==17282==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
==17282==    by 0x1233DEB: av_log_default_callback (log.c:397)
==17282==    by 0x1234092: av_vlog (log.c:432)
==17282==    by 0x1233EF1: av_log (log.c:411)
==17282==    by 0x830E9D: cfhd_decode (cfhd.c:897)
==17282==    by 0x860064: decode_simple_internal (decode.c:327)
==17282==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
==17282==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
==17282==    by 0x861019: avcodec_send_packet (decode.c:608)
==17282==    by 0x2525A7: decode (ffmpeg.c:2285)

[...]
Michael Niedermayer April 1, 2021, 10:49 p.m. UTC | #11 
On Fri, Apr 02, 2021 at 12:25:53AM +0200, Michael Niedermayer wrote:
> On Thu, Apr 01, 2021 at 09:22:23PM +0200, Paul B Mahol wrote:
> > Try this attached patch. I have not looked at all samples, as some allocate
> > too much memory for my system.
> 
> > But this patch points where real bugs are, unlike yours patch which hides
> > real bugs even more.
> 
> I would appreciate if cfhd wouldnt have so many real bugs.
> Your approach seems to be to fix what the fuzzer finds. What my patch was
> moving toward is to make the code more secure and robust not to fix individual
> bugs. My patch was never intended to be the end of such improvment, but with
> the first stage being rejected iam of course not putting time in the next ...
> 
> but thats not so importrant now, whats important is the bugs here
> and your patch eliminates all of the current group but one. Thats good!
> Heres what remains:
> ffmpeg -threads 1 -i dec_fuzzer-30739.nut -f null -

correction, the fuzzer found an alternative sample for 29754 which still crashes
this seems to also use less memory than the other remaining sample
will send the sample privatly

[cfhd @ 0x16d92180] Invalid lowpass height
==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
==24087==    by 0x1234092: av_vlog (log.c:432)
==24087==    by 0x1233EF1: av_log (log.c:411)
==24087==    by 0x82FCFB: cfhd_decode (cfhd.c:721)
==24087==    by 0x860064: decode_simple_internal (decode.c:327)
==24087==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
==24087==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
==24087==    by 0x861019: avcodec_send_packet (decode.c:608)
==24087==    by 0x2525A7: decode (ffmpeg.c:2285)
==24087==    by 0x252DC7: decode_video (ffmpeg.c:2425)
==24087==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
Error while decoding stream #0:0: Invalid argument
==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
==24087==    by 0x1234092: av_vlog (log.c:432)
==24087==    by 0x1233EF1: av_log (log.c:411)
==24087==    by 0x254285: process_input_packet (ffmpeg.c:2718)
==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
[cfhd @ 0x16d92180] Invalid lowpass height
==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
==24087==    by 0x1234092: av_vlog (log.c:432)
==24087==    by 0x1233EF1: av_log (log.c:411)
==24087==    by 0x82FCFB: cfhd_decode (cfhd.c:721)
==24087==    by 0x860064: decode_simple_internal (decode.c:327)
==24087==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
==24087==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
==24087==    by 0x861019: avcodec_send_packet (decode.c:608)
==24087==    by 0x2525A7: decode (ffmpeg.c:2285)
==24087==    by 0x252DC7: decode_video (ffmpeg.c:2425)
==24087==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
Error while decoding stream #0:0: Invalid argument
==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
==24087==    by 0x1234092: av_vlog (log.c:432)
==24087==    by 0x1233EF1: av_log (log.c:411)
==24087==    by 0x254285: process_input_packet (ffmpeg.c:2718)
==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
[cfhd @ 0x16d92180] Sample format of 1039 is not implemented. Update your FFmpeg version to the newest one from Git. If the problem still occurs, it means that your file has a feature which has not been implemented.
Error while decoding stream #0:0: Not yet implemented in FFmpeg, patches welcome
==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
==24087==    by 0x1234092: av_vlog (log.c:432)
==24087==    by 0x1233EF1: av_log (log.c:411)
==24087==    by 0x254285: process_input_packet (ffmpeg.c:2718)
==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
[cfhd @ 0x16d92180] Invalid lowpass height
==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
==24087==    by 0x1234092: av_vlog (log.c:432)
==24087==    by 0x1233EF1: av_log (log.c:411)
==24087==    by 0x82FCFB: cfhd_decode (cfhd.c:721)
==24087==    by 0x860064: decode_simple_internal (decode.c:327)
==24087==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
==24087==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
==24087==    by 0x861019: avcodec_send_packet (decode.c:608)
==24087==    by 0x2525A7: decode (ffmpeg.c:2285)
==24087==    by 0x252DC7: decode_video (ffmpeg.c:2425)
==24087==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
Error while decoding stream #0:0: Invalid argument
==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
==24087==    by 0x1234092: av_vlog (log.c:432)
==24087==    by 0x1233EF1: av_log (log.c:411)
==24087==    by 0x254285: process_input_packet (ffmpeg.c:2718)
==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
==24087== Invalid read of size 16
==24087==    at 0x10A1385: ??? (libavcodec/x86/cfhddsp.asm:384)
==24087==    by 0x1FFEFFF74F: ???
==24087==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==24087== 
==24087== 
==24087== Process terminating with default action of signal 11 (SIGSEGV)
==24087==  Access not within mapped region at address 0x0
==24087==    at 0x10A1385: ??? (libavcodec/x86/cfhddsp.asm:384)
==24087==    by 0x1FFEFFF74F: ???
==24087==  If you believe this happened as a result of a stack
==24087==  overflow in your program's main thread (unlikely but
==24087==  possible), you can try to increase the size of the
==24087==  main thread stack using the --main-stacksize= flag.
==24087==  The main thread stack size used in this run was 8388608.
==24087== 
==24087== HEAP SUMMARY:
==24087==     in use at exit: 4,909,751 bytes in 242 blocks
==24087==   total heap usage: 1,961 allocs, 1,719 frees, 23,859,585 bytes allocated
==24087== 
==24087== 11,776 bytes in 32 blocks are possibly lost in loss record 174 of 181
==24087==    at 0x4C33B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==24087==    by 0x4013646: allocate_dtv (dl-tls.c:286)
==24087==    by 0x4013646: _dl_allocate_tls (dl-tls.c:530)
==24087==    by 0xCB4F227: allocate_stack (allocatestack.c:627)
==24087==    by 0xCB4F227: pthread_create@@GLIBC_2.2.5 (pthread_create.c:644)
==24087==    by 0x12669C2: avpriv_slicethread_create (slicethread.c:147)
==24087==    by 0x2BC153: thread_init_internal (pthread.c:78)
==24087==    by 0x2BC1F1: ff_graph_thread_init (pthread.c:97)
==24087==    by 0x29FE2E: avfilter_graph_alloc_filter (avfiltergraph.c:180)
==24087==    by 0x2BA603: create_filter (graphparser.c:132)
==24087==    by 0x2BA896: parse_filter (graphparser.c:201)
==24087==    by 0x2BB171: avfilter_graph_parse2 (graphparser.c:438)
==24087==    by 0x240FD9: configure_filtergraph (ffmpeg_filter.c:1034)
==24087==    by 0x2523A2: ifilter_send_frame (ffmpeg.c:2234)
==24087==    by 0x2526DA: send_frame_to_filters (ffmpeg.c:2315)
==24087==    by 0x25348B: decode_video (ffmpeg.c:2512)
==24087==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
==24087== 
==24087== LEAK SUMMARY:
==24087==    definitely lost: 0 bytes in 0 blocks
==24087==    indirectly lost: 0 bytes in 0 blocks
==24087==      possibly lost: 11,776 bytes in 32 blocks
==24087==    still reachable: 4,897,975 bytes in 210 blocks
==24087==         suppressed: 0 bytes in 0 blocks
==24087== Reachable blocks (those to which a pointer was found) are not shown.
==24087== To see them, rerun with: --leak-check=full --show-leak-kinds=all





[...]
Michael Niedermayer April 1, 2021, 10:53 p.m. UTC | #12 
On Fri, Apr 02, 2021 at 12:49:26AM +0200, Michael Niedermayer wrote:
> On Fri, Apr 02, 2021 at 12:25:53AM +0200, Michael Niedermayer wrote:
> > On Thu, Apr 01, 2021 at 09:22:23PM +0200, Paul B Mahol wrote:
> > > Try this attached patch. I have not looked at all samples, as some allocate
> > > too much memory for my system.
> > 
> > > But this patch points where real bugs are, unlike yours patch which hides
> > > real bugs even more.
> > 
> > I would appreciate if cfhd wouldnt have so many real bugs.
> > Your approach seems to be to fix what the fuzzer finds. What my patch was
> > moving toward is to make the code more secure and robust not to fix individual
> > bugs. My patch was never intended to be the end of such improvment, but with
> > the first stage being rejected iam of course not putting time in the next ...
> > 
> > but thats not so importrant now, whats important is the bugs here
> > and your patch eliminates all of the current group but one. Thats good!
> > Heres what remains:
> > ffmpeg -threads 1 -i dec_fuzzer-30739.nut -f null -
> 
> correction, the fuzzer found an alternative sample for 29754 which still crashes
> this seems to also use less memory than the other remaining sample
> will send the sample privatly
> 
> [cfhd @ 0x16d92180] Invalid lowpass height
> ==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
> ==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
> ==24087==    by 0x1234092: av_vlog (log.c:432)
> ==24087==    by 0x1233EF1: av_log (log.c:411)
> ==24087==    by 0x82FCFB: cfhd_decode (cfhd.c:721)
> ==24087==    by 0x860064: decode_simple_internal (decode.c:327)
> ==24087==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
> ==24087==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
> ==24087==    by 0x861019: avcodec_send_packet (decode.c:608)
> ==24087==    by 0x2525A7: decode (ffmpeg.c:2285)
> ==24087==    by 0x252DC7: decode_video (ffmpeg.c:2425)
> ==24087==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
> ==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
> ==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
> ==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
> ==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
> Error while decoding stream #0:0: Invalid argument
> ==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
> ==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
> ==24087==    by 0x1234092: av_vlog (log.c:432)
> ==24087==    by 0x1233EF1: av_log (log.c:411)
> ==24087==    by 0x254285: process_input_packet (ffmpeg.c:2718)
> ==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
> ==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
> ==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
> ==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
> [cfhd @ 0x16d92180] Invalid lowpass height
> ==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
> ==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
> ==24087==    by 0x1234092: av_vlog (log.c:432)
> ==24087==    by 0x1233EF1: av_log (log.c:411)
> ==24087==    by 0x82FCFB: cfhd_decode (cfhd.c:721)
> ==24087==    by 0x860064: decode_simple_internal (decode.c:327)
> ==24087==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
> ==24087==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
> ==24087==    by 0x861019: avcodec_send_packet (decode.c:608)
> ==24087==    by 0x2525A7: decode (ffmpeg.c:2285)
> ==24087==    by 0x252DC7: decode_video (ffmpeg.c:2425)
> ==24087==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
> ==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
> ==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
> ==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
> ==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
> Error while decoding stream #0:0: Invalid argument
> ==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
> ==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
> ==24087==    by 0x1234092: av_vlog (log.c:432)
> ==24087==    by 0x1233EF1: av_log (log.c:411)
> ==24087==    by 0x254285: process_input_packet (ffmpeg.c:2718)
> ==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
> ==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
> ==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
> ==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
> [cfhd @ 0x16d92180] Sample format of 1039 is not implemented. Update your FFmpeg version to the newest one from Git. If the problem still occurs, it means that your file has a feature which has not been implemented.
> Error while decoding stream #0:0: Not yet implemented in FFmpeg, patches welcome
> ==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
> ==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
> ==24087==    by 0x1234092: av_vlog (log.c:432)
> ==24087==    by 0x1233EF1: av_log (log.c:411)
> ==24087==    by 0x254285: process_input_packet (ffmpeg.c:2718)
> ==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
> ==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
> ==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
> ==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
> [cfhd @ 0x16d92180] Invalid lowpass height
> ==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
> ==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
> ==24087==    by 0x1234092: av_vlog (log.c:432)
> ==24087==    by 0x1233EF1: av_log (log.c:411)
> ==24087==    by 0x82FCFB: cfhd_decode (cfhd.c:721)
> ==24087==    by 0x860064: decode_simple_internal (decode.c:327)
> ==24087==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
> ==24087==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
> ==24087==    by 0x861019: avcodec_send_packet (decode.c:608)
> ==24087==    by 0x2525A7: decode (ffmpeg.c:2285)
> ==24087==    by 0x252DC7: decode_video (ffmpeg.c:2425)
> ==24087==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
> ==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
> ==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
> ==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
> ==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
> Error while decoding stream #0:0: Invalid argument
> ==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
> ==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
> ==24087==    by 0x1234092: av_vlog (log.c:432)
> ==24087==    by 0x1233EF1: av_log (log.c:411)
> ==24087==    by 0x254285: process_input_packet (ffmpeg.c:2718)
> ==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
> ==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
> ==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
> ==24087==    by 0x25CB3F: main (ffmpeg.c:5005)

> ==24087== Invalid read of size 16
> ==24087==    at 0x10A1385: ??? (libavcodec/x86/cfhddsp.asm:384)
> ==24087==    by 0x1FFEFFF74F: ???
> ==24087==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

without asm:
==24138== Invalid read of size 2
==24138==    at 0x835536: filter (cfhddsp.c:36)
==24138==    by 0x835A68: vert_filter (cfhddsp.c:74)
==24138==    by 0x8333AE: cfhd_decode (cfhd.c:1172)
==24138==    by 0x860064: decode_simple_internal (decode.c:327)
==24138==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
==24138==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
==24138==    by 0x861019: avcodec_send_packet (decode.c:608)
==24138==    by 0x2525A7: decode (ffmpeg.c:2285)
==24138==    by 0x252DC7: decode_video (ffmpeg.c:2425)
==24138==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
==24138==    by 0x25BB79: process_input (ffmpeg.c:4606)
==24138==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
==24138==    by 0x25C1D5: transcode (ffmpeg.c:4800)
==24138==    by 0x25CB3F: main (ffmpeg.c:5005)
==24138==  Address 0x0 is not stack'd, malloc'd or (recently) free'd


[...]
Paul B Mahol April 2, 2021, 3:40 p.m. UTC | #13 
I do not have time or motivation to deal with this and similar issues.

But applying band-aid solutions are not step forward.

On Fri, Apr 2, 2021 at 12:53 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Fri, Apr 02, 2021 at 12:49:26AM +0200, Michael Niedermayer wrote:
> > On Fri, Apr 02, 2021 at 12:25:53AM +0200, Michael Niedermayer wrote:
> > > On Thu, Apr 01, 2021 at 09:22:23PM +0200, Paul B Mahol wrote:
> > > > Try this attached patch. I have not looked at all samples, as some
> allocate
> > > > too much memory for my system.
> > >
> > > > But this patch points where real bugs are, unlike yours patch which
> hides
> > > > real bugs even more.
> > >
> > > I would appreciate if cfhd wouldnt have so many real bugs.
> > > Your approach seems to be to fix what the fuzzer finds. What my patch
> was
> > > moving toward is to make the code more secure and robust not to fix
> individual
> > > bugs. My patch was never intended to be the end of such improvment,
> but with
> > > the first stage being rejected iam of course not putting time in the
> next ...
> > >
> > > but thats not so importrant now, whats important is the bugs here
> > > and your patch eliminates all of the current group but one. Thats good!
> > > Heres what remains:
> > > ffmpeg -threads 1 -i dec_fuzzer-30739.nut -f null -
> >
> > correction, the fuzzer found an alternative sample for 29754 which still
> crashes
> > this seems to also use less memory than the other remaining sample
> > will send the sample privatly
> >
> > [cfhd @ 0x16d92180] Invalid lowpass height
> > ==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
> > ==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
> > ==24087==    by 0x1234092: av_vlog (log.c:432)
> > ==24087==    by 0x1233EF1: av_log (log.c:411)
> > ==24087==    by 0x82FCFB: cfhd_decode (cfhd.c:721)
> > ==24087==    by 0x860064: decode_simple_internal (decode.c:327)
> > ==24087==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
> > ==24087==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
> > ==24087==    by 0x861019: avcodec_send_packet (decode.c:608)
> > ==24087==    by 0x2525A7: decode (ffmpeg.c:2285)
> > ==24087==    by 0x252DC7: decode_video (ffmpeg.c:2425)
> > ==24087==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
> > ==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
> > ==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
> > ==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
> > ==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
> > Error while decoding stream #0:0: Invalid argument
> > ==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
> > ==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
> > ==24087==    by 0x1234092: av_vlog (log.c:432)
> > ==24087==    by 0x1233EF1: av_log (log.c:411)
> > ==24087==    by 0x254285: process_input_packet (ffmpeg.c:2718)
> > ==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
> > ==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
> > ==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
> > ==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
> > [cfhd @ 0x16d92180] Invalid lowpass height
> > ==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
> > ==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
> > ==24087==    by 0x1234092: av_vlog (log.c:432)
> > ==24087==    by 0x1233EF1: av_log (log.c:411)
> > ==24087==    by 0x82FCFB: cfhd_decode (cfhd.c:721)
> > ==24087==    by 0x860064: decode_simple_internal (decode.c:327)
> > ==24087==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
> > ==24087==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
> > ==24087==    by 0x861019: avcodec_send_packet (decode.c:608)
> > ==24087==    by 0x2525A7: decode (ffmpeg.c:2285)
> > ==24087==    by 0x252DC7: decode_video (ffmpeg.c:2425)
> > ==24087==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
> > ==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
> > ==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
> > ==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
> > ==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
> > Error while decoding stream #0:0: Invalid argument
> > ==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
> > ==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
> > ==24087==    by 0x1234092: av_vlog (log.c:432)
> > ==24087==    by 0x1233EF1: av_log (log.c:411)
> > ==24087==    by 0x254285: process_input_packet (ffmpeg.c:2718)
> > ==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
> > ==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
> > ==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
> > ==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
> > [cfhd @ 0x16d92180] Sample format of 1039 is not implemented. Update
> your FFmpeg version to the newest one from Git. If the problem still
> occurs, it means that your file has a feature which has not been
> implemented.
> > Error while decoding stream #0:0: Not yet implemented in FFmpeg, patches
> welcome
> > ==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
> > ==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
> > ==24087==    by 0x1234092: av_vlog (log.c:432)
> > ==24087==    by 0x1233EF1: av_log (log.c:411)
> > ==24087==    by 0x254285: process_input_packet (ffmpeg.c:2718)
> > ==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
> > ==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
> > ==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
> > ==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
> > [cfhd @ 0x16d92180] Invalid lowpass height
> > ==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
> > ==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
> > ==24087==    by 0x1234092: av_vlog (log.c:432)
> > ==24087==    by 0x1233EF1: av_log (log.c:411)
> > ==24087==    by 0x82FCFB: cfhd_decode (cfhd.c:721)
> > ==24087==    by 0x860064: decode_simple_internal (decode.c:327)
> > ==24087==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
> > ==24087==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
> > ==24087==    by 0x861019: avcodec_send_packet (decode.c:608)
> > ==24087==    by 0x2525A7: decode (ffmpeg.c:2285)
> > ==24087==    by 0x252DC7: decode_video (ffmpeg.c:2425)
> > ==24087==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
> > ==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
> > ==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
> > ==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
> > ==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
> > Error while decoding stream #0:0: Invalid argument
> > ==24087==    at 0x123322D: VALGRIND_PRINTF_BACKTRACE (valgrind.h:6303)
> > ==24087==    by 0x1233DEB: av_log_default_callback (log.c:397)
> > ==24087==    by 0x1234092: av_vlog (log.c:432)
> > ==24087==    by 0x1233EF1: av_log (log.c:411)
> > ==24087==    by 0x254285: process_input_packet (ffmpeg.c:2718)
> > ==24087==    by 0x25BB79: process_input (ffmpeg.c:4606)
> > ==24087==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
> > ==24087==    by 0x25C1D5: transcode (ffmpeg.c:4800)
> > ==24087==    by 0x25CB3F: main (ffmpeg.c:5005)
>
> > ==24087== Invalid read of size 16
> > ==24087==    at 0x10A1385: ??? (libavcodec/x86/cfhddsp.asm:384)
> > ==24087==    by 0x1FFEFFF74F: ???
> > ==24087==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
>
> without asm:
> ==24138== Invalid read of size 2
> ==24138==    at 0x835536: filter (cfhddsp.c:36)
> ==24138==    by 0x835A68: vert_filter (cfhddsp.c:74)
> ==24138==    by 0x8333AE: cfhd_decode (cfhd.c:1172)
> ==24138==    by 0x860064: decode_simple_internal (decode.c:327)
> ==24138==    by 0x860C9B: decode_simple_receive_frame (decode.c:526)
> ==24138==    by 0x860D95: decode_receive_frame_internal (decode.c:546)
> ==24138==    by 0x861019: avcodec_send_packet (decode.c:608)
> ==24138==    by 0x2525A7: decode (ffmpeg.c:2285)
> ==24138==    by 0x252DC7: decode_video (ffmpeg.c:2425)
> ==24138==    by 0x253EF3: process_input_packet (ffmpeg.c:2672)
> ==24138==    by 0x25BB79: process_input (ffmpeg.c:4606)
> ==24138==    by 0x25C06D: transcode_step (ffmpeg.c:4746)
> ==24138==    by 0x25C1D5: transcode (ffmpeg.c:4800)
> ==24138==    by 0x25CB3F: main (ffmpeg.c:5005)
> ==24138==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
>
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
> _______________________________________________
> 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".
Michael Niedermayer April 3, 2021, 2:40 p.m. UTC | #14 
On Thu, Apr 01, 2021 at 09:22:23PM +0200, Paul B Mahol wrote:
> Try this attached patch. I have not looked at all samples, as some allocate
> too much memory for my system.
> But this patch points where real bugs are, unlike yours patch which hides
> real bugs even more.

>  cfhd.c |    7 ++++++-
>  cfhd.h |    1 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
> eef066aa3ee9d301ae412809e0ca0bea8cee2c68  0001-avcodec-cfhd-fix-some-crashes-caused-by-excessive-fu.patch
> From fc4abcc0d0058ea8a7cd79ce26bfbcbed4cf5329 Mon Sep 17 00:00:00 2001
> From: Paul B Mahol <onemda@gmail.com>
> Date: Thu, 1 Apr 2021 21:17:17 +0200
> Subject: [PATCH] avcodec/cfhd: fix some crashes caused by excessive fuzzing
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavcodec/cfhd.c | 7 ++++++-
>  libavcodec/cfhd.h | 1 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/cfhd.c b/libavcodec/cfhd.c
> index 1f2ee853c1..e126eb6ac7 100644
> --- a/libavcodec/cfhd.c
> +++ b/libavcodec/cfhd.c
[...]
> @@ -244,6 +246,7 @@ static int alloc_buffers(AVCodecContext *avctx)
>  
>      if ((ret = ff_set_dimensions(avctx, s->coded_width, s->coded_height)) < 0)
>          return ret;
> +    avctx->coded_width = FFALIGN(s->coded_width, 64) + 256;
>      avctx->pix_fmt = s->coded_format;
>  
>      ff_cfhddsp_init(&s->dsp, s->bpc, avctx->pix_fmt == AV_PIX_FMT_BAYER_RGGB16);
[...]
> @@ -665,6 +669,7 @@ static int cfhd_decode(AVCodecContext *avctx, void *data, int *got_frame,
>              ret = ff_set_dimensions(avctx, s->coded_width, s->coded_height);
>              if (ret < 0)
>                  return ret;
> +            avctx->coded_width = FFALIGN(s->coded_width, 64) + 256;
>              if (s->cropped_height) {
>                  unsigned height = s->cropped_height << (avctx->pix_fmt == AV_PIX_FMT_BAYER_RGGB16);
>                  if (avctx->height < height)

Please document why coded_width has this extra alignment and padding added
Also if these are still needed after the patchset i just posted then please
apply

thanks

[...]
diff mbox series

Patch

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;