diff mbox series

[FFmpeg-devel,2/2] avcodec/cfhd: More strictly check tag order

Message ID 20200829204428.11064-2-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/2] avcodec/cfhd: Replace a few literal numbers by named constants | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Michael Niedermayer Aug. 29, 2020, 8:44 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.

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/cfhd.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++
 libavcodec/cfhd.h |  3 +++
 2 files changed, 69 insertions(+)

Comments

Paul B Mahol Aug. 29, 2020, 9 p.m. UTC | #1 
On 8/29/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> This is based on the encoder and a small number of CFHD sample files
> It should make the decoder more robust against crafted input.
> Due to the lack of a proper specification it is possible that this
> may be too strict and may need to be tuned as files not following this
> ordering are found.
>

Breaks decoding of this and similar files:

https://samples.ffmpeg.org/V-codecs/CFHD/MT_BeartoothHighway_1min_Cineform.avi

It is too strict as our encoder does not implement 3d transforms.
Michael Niedermayer Aug. 29, 2020, 10:25 p.m. UTC | #2 
On Sat, Aug 29, 2020 at 11:00:13PM +0200, Paul B Mahol wrote:
> On 8/29/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > This is based on the encoder and a small number of CFHD sample files
> > It should make the decoder more robust against crafted input.
> > Due to the lack of a proper specification it is possible that this
> > may be too strict and may need to be tuned as files not following this
> > ordering are found.
> >
> 
> Breaks decoding of this and similar files:
> 
> https://samples.ffmpeg.org/V-codecs/CFHD/MT_BeartoothHighway_1min_Cineform.avi
> 
> It is too strict as our encoder does not implement 3d transforms.

fixed this and the othert files in that directory (will post a new patch
in a moment)

also subsequently after this patch somethig to check not just for
fields not being in the wrong place we also should probably check for
mandatory fields being not missing.
I assume for this too guessing from files & sdk & any other implementation
is the only option we have

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/cfhd.c b/libavcodec/cfhd.c
index ea35f03869..1e88e651ae 100644
--- a/libavcodec/cfhd.c
+++ b/libavcodec/cfhd.c
@@ -361,6 +361,64 @@  static int alloc_buffers(AVCodecContext *avctx)
     return 0;
 }
 
+static int handle_tag_order(CFHDContext *s, int tag, int data)
+{
+    int atag = abs(tag);
+    // We do not restrict tags outside the enum
+    if (atag >= LastTag)
+        return 0;
+
+    if (s->previous_marker == 0x88888888) {
+        if (tag >= 1 && tag <= 23 || atag >= 63 && atag <= 66 || atag >= 68 && atag <= 71 || atag >= 79 && atag <= 81 || atag >= 83 && atag <= 85 || atag >= 91 && atag <= 93) {
+            ;
+        } else if (tag == BitstreamMarker && data == 0x1a4a) {
+            ;
+        } else
+            return AVERROR_INVALIDDATA;
+    } else if (s->previous_marker == 0x1a4a) {
+        if (tag >= 25 && tag <= 36) {
+            ;
+        } else if (tag == BitstreamMarker && data == 0xf0f) {
+            ;
+        } else
+            return AVERROR_INVALIDDATA;
+    } else if (s->previous_marker == 0xf0f) {
+        if (tag != BitstreamMarker || data != 0x1b4b)
+            return AVERROR_INVALIDDATA;
+    } else if (s->previous_marker == 0x1b4b) {
+        if (tag != BitstreamMarker || data != 0xd0d)
+            return AVERROR_INVALIDDATA;
+    } else if (s->previous_marker == 0xd0d) {
+        if (tag >= 37 && tag <= 47) {
+            ;
+        } else if (tag == BitstreamMarker && data == 0xe0e) {
+            ;
+        } else
+            return AVERROR_INVALIDDATA;
+    } else if (s->previous_marker == 0xe0e) {
+        if (tag >= 48 && tag <= 56 || tag == BandCodingFlags) {
+            ;
+        } else if (-tag >= 74 && -tag <= 76) {
+            ;
+        } else if (tag == BitstreamMarker && (data == 0xc0c || data == 0xe0e)) {
+            ;
+        } else
+            return AVERROR_INVALIDDATA;
+    } else if (s->previous_marker == 0xc0c) {
+        if (tag == 1 || tag == 24 || tag == 62) {
+            ;
+        } else if (tag == BitstreamMarker && (data == 0xd0d || data == 0x1a4a)) {
+            ;
+        } else
+            return AVERROR_INVALIDDATA;
+    } else
+        return AVERROR_INVALIDDATA;
+
+    if (tag == BitstreamMarker)
+        s->previous_marker = data;
+    return 0;
+}
+
 static int cfhd_decode(AVCodecContext *avctx, void *data, int *got_frame,
                        AVPacket *avpkt)
 {
@@ -374,6 +432,7 @@  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 = 0x88888888;
 
     bytestream2_init(&gb, avpkt->data, avpkt->size);
 
@@ -385,6 +444,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..c46ab01c17 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,8 @@  typedef struct CFHDContext {
     Plane plane[4];
     Peak peak;
 
+    int previous_marker;
+
     CFHDDSPContext dsp;
 } CFHDContext;