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()
Related show

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.
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;