From bfc4fb532b340d62eb4cd1db5d4b93a273f95b6c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <tjoppen@acc.umu.se>
Date: Sun, 18 Aug 2019 17:42:27 +0200
Subject: [PATCH] libavcodec/cinepak: Separate decoding from parsing
This is rather strict and replaces the previous hacks with checks that should hopefully placate the fuzzers for the forseeable future
---
libavcodec/cinepak.c | 268 +++++++++++++++++++++++++-----------
tests/ref/fate/cvid-palette | 1 -
tests/ref/fate/cvid-partial | 1 -
3 files changed, 191 insertions(+), 79 deletions(-)
@@ -46,6 +46,8 @@
typedef uint8_t cvid_codebook[12];
#define MAX_STRIPS 32
+#define MAX_HEIGHT UINT16_MAX
+#define MB_AREA 16
typedef struct cvid_strip {
uint16_t id;
@@ -53,6 +55,8 @@ typedef struct cvid_strip {
uint16_t x2, y2;
cvid_codebook v4_codebook[256];
cvid_codebook v1_codebook[256];
+ const uint8_t *data; // points to strip data, excluding header
+ int size;
} cvid_strip;
typedef struct CinepakContext {
@@ -60,9 +64,6 @@ typedef struct CinepakContext {
AVCodecContext *avctx;
AVFrame *frame;
- const unsigned char *data;
- int size;
-
int width, height;
int palette_video;
@@ -71,6 +72,10 @@ typedef struct CinepakContext {
int sega_film_skip_bytes;
uint32_t pal[256];
+
+ int frame_flags;
+ int num_strips;
+ uint8_t strip_cover[MAX_HEIGHT];
} CinepakContext;
static void cinepak_decode_codebook (cvid_codebook *codebook,
@@ -269,12 +274,6 @@ static int cinepak_decode_strip (CinepakContext *s,
const uint8_t *eod = (data + size);
int chunk_id, chunk_size;
- /* coordinate sanity checks */
- if (strip->x2 > s->width ||
- strip->y2 > s->height ||
- strip->x1 >= strip->x2 || strip->y1 >= strip->y2)
- return AVERROR_INVALIDDATA;
-
while ((data + 4) <= eod) {
chunk_id = data[0];
chunk_size = AV_RB24 (&data[1]) - 4;
@@ -315,16 +314,20 @@ static int cinepak_decode_strip (CinepakContext *s,
return AVERROR_INVALIDDATA;
}
-static int cinepak_predecode_check (CinepakContext *s)
+static int cinepak_parse_header(AVCodecContext *avctx, const AVPacket *avpkt)
{
- int num_strips;
- int encoded_buf_size;
-
- num_strips = AV_RB16 (&s->data[8]);
- encoded_buf_size = AV_RB24(&s->data[1]);
+ CinepakContext *s = avctx->priv_data;
+ int w, h, encoded_buf_size;
- if (s->size < encoded_buf_size * (int64_t)(100 - s->avctx->discard_damaged_percentage) / 100)
+ if (avpkt->size < 10) {
return AVERROR_INVALIDDATA;
+ }
+
+ s->frame_flags = avpkt->data[0];
+ encoded_buf_size = AV_RB24(&avpkt->data[1]);
+ w = AV_RB16(&avpkt->data[4]);
+ h = AV_RB16(&avpkt->data[6]);
+ s->num_strips = AV_RB16(&avpkt->data[8]);
/* if this is the first frame, check for deviant Sega FILM data */
if (s->sega_film_skip_bytes == -1) {
@@ -332,20 +335,20 @@ static int cinepak_predecode_check (CinepakContext *s)
avpriv_request_sample(s->avctx, "encoded_buf_size 0");
return AVERROR_PATCHWELCOME;
}
- if (encoded_buf_size != s->size && (s->size % encoded_buf_size) != 0) {
+ if (encoded_buf_size != avpkt->size && (avpkt->size % encoded_buf_size) != 0) {
/* If the encoded frame size differs from the frame size as indicated
* by the container file, this data likely comes from a Sega FILM/CPK file.
* If the frame header is followed by the bytes FE 00 00 06 00 00 then
* this is probably one of the two known files that have 6 extra bytes
* after the frame header. Else, assume 2 extra bytes. The container
* size also cannot be a multiple of the encoded size. */
- if (s->size >= 16 &&
- (s->data[10] == 0xFE) &&
- (s->data[11] == 0x00) &&
- (s->data[12] == 0x00) &&
- (s->data[13] == 0x06) &&
- (s->data[14] == 0x00) &&
- (s->data[15] == 0x00))
+ if (avpkt->size >= 16 &&
+ (avpkt->data[10] == 0xFE) &&
+ (avpkt->data[11] == 0x00) &&
+ (avpkt->data[12] == 0x00) &&
+ (avpkt->data[13] == 0x06) &&
+ (avpkt->data[14] == 0x00) &&
+ (avpkt->data[15] == 0x00))
s->sega_film_skip_bytes = 6;
else
s->sega_film_skip_bytes = 2;
@@ -353,70 +356,188 @@ static int cinepak_predecode_check (CinepakContext *s)
s->sega_film_skip_bytes = 0;
}
- if (s->size < 10 + s->sega_film_skip_bytes + num_strips * 12)
+ // compare w and h to the width and height in CinepakContext, not AVCodecContext
+ // the former has been appropriately rounded up
+ if (w != s->width || h != s->height) {
+ av_log(avctx, AV_LOG_ERROR, "dimension mismatch (%ix%i vs %ix%i)\n", w, h, avctx->width, avctx->height);
return AVERROR_INVALIDDATA;
+ }
- if (num_strips) {
- const uint8_t *data = s->data + 10 + s->sega_film_skip_bytes;
- int strip_size = AV_RB24 (data + 1);
- if (strip_size < 12 || strip_size > encoded_buf_size)
- return AVERROR_INVALIDDATA;
+ if (s->num_strips == 0) {
+ // the old code would accept num_strips == 0 if there was at least a palette change
+ // I know of no sample that requires this, and the VfW 1.1 decoder
+ // will produce garbage if the entire frame isn't covered by strips
+ av_log(avctx, AV_LOG_ERROR, "num_strips == 0\n");
+ return AVERROR_INVALIDDATA;
+ }
+
+ // unfortunately we can't demand that the sizes match exactly
+ // example: http://samples.mplayerhq.hu/V-codecs/CVID/bad_cinepak_frame_size.mov
+ // at least require that the header value is no larger than the size of the packet
+ if (encoded_buf_size + s->sega_film_skip_bytes > avpkt->size) {
+ av_log(avctx, AV_LOG_ERROR, "CVID size larger than actual packet: %i vs %i\n", encoded_buf_size + s->sega_film_skip_bytes, avpkt->size);
+ return AVERROR_INVALIDDATA;
+ }
+
+ if (s->sega_film_skip_bytes) {
+ int skip_diff = avpkt->size - encoded_buf_size - s->sega_film_skip_bytes;
+ if (skip_diff != 2 && skip_diff != 6) {
+ // there doesn't seem to be much pattern to these 2 or 6 last bytes
+ // getting samples with different skip_diffs might be useful for further RE work
+ avpriv_request_sample(s->avctx, "SEGA FILM file with skip_diff %i not equal to 2 or 6", skip_diff);
+ return AVERROR_PATCHWELCOME;
+ }
}
return 0;
}
-static int cinepak_decode (CinepakContext *s)
+static int cinepak_parse_strip_headers(AVCodecContext *avctx, const AVPacket *avpkt)
{
- const uint8_t *eod = (s->data + s->size);
- int i, result, strip_size, frame_flags, num_strips;
- int y0 = 0;
+ CinepakContext *s = avctx->priv_data;
+ const uint8_t *ptr = avpkt->data + 10 + s->sega_film_skip_bytes;
+ int i, y0 = 0, bytes_left = avpkt->size, htot = 0;
- frame_flags = s->data[0];
- num_strips = AV_RB16 (&s->data[8]);
+ if (s->num_strips > MAX_STRIPS) {
+ avpriv_request_sample(avctx, "num_strips %i > %i", s->num_strips, MAX_STRIPS);
+ return AVERROR_PATCHWELCOME;
+ }
- s->data += 10 + s->sega_film_skip_bytes;
+ // the frame is a keyframe only if none of the strips are inter coded
+ s->frame->key_frame = 1;
- num_strips = FFMIN(num_strips, MAX_STRIPS);
+ memset(s->strip_cover, 0, s->height);
- s->frame->key_frame = 0;
+ for (i = 0; i < s->num_strips; i++) {
+ int id, w, h, pixels;
- for (i=0; i < num_strips; i++) {
- if ((s->data + 12) > eod)
- return AVERROR_INVALIDDATA;
+ if (bytes_left < 12) {
+ avpriv_request_sample(avctx, "only %i bytes left for strip %i header", bytes_left, i);
+ return AVERROR_PATCHWELCOME;
+ }
+
+ id = s->strips[i].id = ptr[0];
+
+ if (id != 0x10 && id != 0x11 &&
+ // http://samples.mplayerhq.hu/V-codecs/CVID/1984.apple_ad.mov
+ id != 0x00 && id != 0x01) {
+ avpriv_request_sample(s->avctx, "unexpected strip ID %02x", s->strips[i].id);
+ return AVERROR_PATCHWELCOME;
+ }
- s->strips[i].id = s->data[0];
/* zero y1 means "relative to the previous stripe" */
- if (!(s->strips[i].y1 = AV_RB16 (&s->data[4])))
- s->strips[i].y2 = (s->strips[i].y1 = y0) + AV_RB16 (&s->data[8]);
+ if (!(s->strips[i].y1 = AV_RB16 (&ptr[4])))
+ s->strips[i].y2 = (s->strips[i].y1 = y0) + AV_RB16 (&ptr[8]);
else
- s->strips[i].y2 = AV_RB16 (&s->data[8]);
- s->strips[i].x1 = AV_RB16 (&s->data[6]);
- s->strips[i].x2 = AV_RB16 (&s->data[10]);
+ s->strips[i].y2 = AV_RB16 (&ptr[8]);
+ s->strips[i].x1 = AV_RB16 (&ptr[6]);
+ s->strips[i].x2 = AV_RB16 (&ptr[10]);
+
+ /* coordinate sanity checks */
+ if (s->strips[i].x2 > s->width ||
+ s->strips[i].y2 > s->height ||
+ s->strips[i].x1 >= s->strips[i].x2 ||
+ s->strips[i].y1 >= s->strips[i].y2) {
+ av_log(avctx, AV_LOG_ERROR, "strip %i with bad coordinates (%i,%i %i,%i)", i,
+ (int)s->strips[i].x1, (int)s->strips[i].y1,
+ (int)s->strips[i].x2, (int)s->strips[i].y2);
+ return AVERROR_INVALIDDATA;
+ }
- if (s->strips[i].id == 0x10)
- s->frame->key_frame = 1;
+ // cinepak.txt claims size is 16-bit, but
+ // http://samples.mplayerhq.hu/V-codecs/CVID/tn.avi
+ // has strips which are bigger than that
+ s->strips[i].size = AV_RB24(&ptr[1]);
- strip_size = AV_RB24 (&s->data[1]) - 12;
- if (strip_size < 0)
+ if (s->strips[i].size < 12 || s->strips[i].size > bytes_left) {
+ av_log(avctx, AV_LOG_ERROR, "invalid strip %i size: %i vs %i\n", i, s->strips[i].size, bytes_left);
return AVERROR_INVALIDDATA;
- s->data += 12;
- strip_size = ((s->data + strip_size) > eod) ? (eod - s->data) : strip_size;
+ }
- if ((i > 0) && !(frame_flags & 0x01)) {
+ w = s->strips[i].x2 - s->strips[i].x1;
+ h = s->strips[i].y2 - s->strips[i].y1;
+ htot += h;
+ pixels = w*h;
+
+ // I'm not sure if vertical splitting is allowed, all samples available use only horizontal splitting
+ // disallow it for now, since allowing it would make the strip coverage check further down more expensive
+ // ideally we should check what both the VfW 1.1 and MacOS decoders do in this case
+ if (w != s->width) {
+ avpriv_request_sample(s->avctx, "strip with %i != frame width %i", w, s->width);
+ return AVERROR_PATCHWELCOME;
+ }
+
+ // id == 0x11 vs id != 0x10? 1984.apple_ad.mov makes this tricky
+ if (id != 0x10) {
+ // inter-coded -> not a keyframe
+ // there can be a mix of intra and inter strips in a single frame
+ s->frame->key_frame = 0;
+
+ // inter strips can be MODE_MC, which are at least 1 bit per MB
+ if (s->strips[i].size < pixels/(MB_AREA*8)) {
+ avpriv_request_sample(s->avctx, "strip %i too small to be inter (%i B vs %i pixels)",
+ i, s->strips[i].size, pixels);
+ return AVERROR_PATCHWELCOME;
+ }
+ } else if (s->strips[i].size < pixels/MB_AREA) {
+ // intra strips are at least 8 bits per MB (MODE_V1_ONLY)
+ // MODE_V1_V4 is 9 bits per MB, but we don't know if the strip is that mode at this stage
+ avpriv_request_sample(s->avctx, "strip %i too small to be intra (%i B vs %i pixels)",
+ i, s->strips[i].size, pixels);
+ return AVERROR_PATCHWELCOME;
+ }
+
+ // mark coded lines
+ for (int y = s->strips[i].y1; y < s->strips[i].y2; y++) {
+ s->strip_cover[y] = 1;
+ }
+
+ // point into the strip data
+ s->strips[i].data = ptr + 12;
+ ptr += s->strips[i].size;
+ bytes_left -= s->strips[i].size;
+ s->strips[i].size -= 12;
+ y0 = s->strips[i].y2;
+ }
+
+ // require that every single line be coded at least once
+ for (int y = 0; y < s->height; y++) {
+ if (!s->strip_cover[y]) {
+ avpriv_request_sample(s->avctx, "line %i not covered by a strip", y);
+ return AVERROR_PATCHWELCOME;
+ }
+ }
+
+ // require that every line be coded *exactly* once
+ if (htot != s->height) {
+ avpriv_request_sample(s->avctx, "strip heights not adding up to frame height");
+ return AVERROR_PATCHWELCOME;
+ }
+
+ return 0;
+}
+
+static int cinepak_decode (CinepakContext *s)
+{
+ int i, result;
+
+ for (i = 0; i < s->num_strips; i++) {
+ // cinepak.txt claims that codebooks should be copied from the last strip of the previous frame
+ // I am not sure what to do in case we have just seeked to a keyframe
+ // this also requires keeping track of which strip was the last one,
+ // which I'm not doing quite yet
+ // the samples we have seem to decode fine the way this is right now, no need to mess with it unless a user complains
+ if ((i > 0) && !(s->frame_flags & 0x01)) {
memcpy (s->strips[i].v4_codebook, s->strips[i-1].v4_codebook,
sizeof(s->strips[i].v4_codebook));
memcpy (s->strips[i].v1_codebook, s->strips[i-1].v1_codebook,
sizeof(s->strips[i].v1_codebook));
}
- result = cinepak_decode_strip (s, &s->strips[i], s->data, strip_size);
+ result = cinepak_decode_strip (s, &s->strips[i], s->strips[i].data, s->strips[i].size);
if (result != 0)
return result;
-
- s->data += strip_size;
- y0 = s->strips[i].y2;
}
return 0;
}
@@ -429,6 +550,13 @@ static av_cold int cinepak_decode_init(AVCodecContext *avctx)
s->width = (avctx->width + 3) & ~3;
s->height = (avctx->height + 3) & ~3;
+ // dimensions are limited to 16-bit in the format,
+ // so it would be exceedingly unlikely that this would ever be hit outside of fuzzing
+ if (s->height > MAX_HEIGHT) {
+ avpriv_request_sample(s->avctx, "height > UINT16_MAX");
+ return AVERROR_PATCHWELCOME;
+ }
+
s->sega_film_skip_bytes = -1; /* uninitialized state */
// check for paletted data
@@ -451,25 +579,11 @@ static int cinepak_decode_frame(AVCodecContext *avctx,
void *data, int *got_frame,
AVPacket *avpkt)
{
- const uint8_t *buf = avpkt->data;
- int ret = 0, buf_size = avpkt->size;
CinepakContext *s = avctx->priv_data;
- int num_strips;
-
- s->data = buf;
- s->size = buf_size;
-
- if (s->size < 10)
- return AVERROR_INVALIDDATA;
-
- num_strips = AV_RB16 (&s->data[8]);
-
- //Empty frame, do not waste time
- if (!num_strips && (!s->palette_video || !av_packet_get_side_data(avpkt, AV_PKT_DATA_PALETTE, NULL)))
- return buf_size;
+ int ret = 0;
- if ((ret = cinepak_predecode_check(s)) < 0) {
- av_log(avctx, AV_LOG_ERROR, "cinepak_predecode_check failed\n");
+ if ((ret = cinepak_parse_header(avctx, avpkt)) < 0 ||
+ (ret = cinepak_parse_strip_headers(avctx, avpkt)) < 0) {
return ret;
}
@@ -500,7 +614,7 @@ static int cinepak_decode_frame(AVCodecContext *avctx,
*got_frame = 1;
/* report that the buffer was completely consumed */
- return buf_size;
+ return avpkt->size;
}
static av_cold int cinepak_decode_end(AVCodecContext *avctx)
@@ -58,4 +58,3 @@
0, 52, 52, 1, 57600, 0xdae585c7
0, 53, 53, 1, 57600, 0xf99baa60
0, 54, 54, 1, 57600, 0x28a8b1ee
-0, 55, 55, 1, 57600, 0xcd5587f8
@@ -81,4 +81,3 @@
0, 75, 75, 1, 224400, 0x920ee561
0, 76, 76, 1, 224400, 0x8a2c1bbf
0, 77, 77, 1, 224400, 0x6150c072
-0, 78, 78, 1, 224400, 0x499dc869
--
2.20.1