From patchwork Tue Sep 3 17:29:53 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Tomas_H=C3=A4rdin?= X-Patchwork-Id: 14886 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id D3F82449364 for ; Tue, 3 Sep 2019 20:30:03 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id AC7526808F1; Tue, 3 Sep 2019 20:30:03 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from tomyris.acc.umu.se (tomyris.acc.umu.se [130.239.18.190]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 40CB568046A for ; Tue, 3 Sep 2019 20:29:57 +0300 (EEST) Received: from localhost (localhost.localdomain [127.0.0.1]) by amavisd-new (Postfix) with ESMTP id 58F4044B8E for ; Tue, 3 Sep 2019 19:29:56 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=acc.umu.se; s=mail1; t=1567531796; bh=cVoXulHqbhVXEUFzYWp5phvehMocPRLkdWdqIlxMPVg=; h=Subject:From:To:Date:In-Reply-To:References:From; b=EEu51dXV84W4qalEHrcSZf+1FviZGPm7HL9o6YvaRhiiDX3RIXV2iPwfmUYhVe7G/ jLyV+Oa3wh/v8iPL5fRJkQk6+UXtmHQj1OCOuC1O5FskwMtE8hMsPnxI1IrSxSwtP3 +F5WXD3B2dW0B/T1PugDuamA4nwMalEE5Y9WMbiQz009zj+48D1y/Fg1dmgVlKID08 WH3kdRWc6kzyXgz/O08uZQux/MTh57cuNr8KN9/gxsArI2qxsLkZwl7ieBJ7Hv4lYT wxxkGc6D0G/ZYV3Gaw8ShC2ndMZHgQTQBuK57IH0TmCHBfpwjd8nfYGakEwZsn02jZ FU64xM8T3LeCQ== Received: from laptop.lan (h-39-105.A258.priv.bahnhof.se [79.136.39.105]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: tjoppen) by tomyris.acc.umu.se (Postfix) with ESMTPSA id 8ED3944B8D for ; Tue, 3 Sep 2019 19:29:54 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=acc.umu.se; s=mail1; t=1567531794; bh=cVoXulHqbhVXEUFzYWp5phvehMocPRLkdWdqIlxMPVg=; h=Subject:From:To:Date:In-Reply-To:References:From; b=JzuyN+9yDP0DTebQpPV8lCyXPOIhjLy83q68tuNZ+4502Ani6lENtBmKGFyhgJZRL SDZLQRXts1SIoxO7CuAQHKzdCeVPawdw+n7wE76IYQ6YdakvyC/MJMqn6Ne4vKOTqA cEB3WbxY6ehEskaJ+KTf64Sb2wgBUC1dnZ0axsTM9lwFq9ELMgupmaef3HJ3yO+xgJ BqhoVrp6JosAFVAEoUoKFu+Fz6JkH4PWaWHbNeLTQPXthrmQ8MiBnKK76ab1dFdGte zCztwsalfOPGaGPOGjKAvwP8NSwlFOl/7YSAgcANHr1vllyevqz9RPBYmrMd9TdEI6 fv9M/IwpWfPgQ== Message-ID: <5c6faf611f4f497af427e9c20c82406d91c7cff6.camel@acc.umu.se> From: Tomas =?ISO-8859-1?Q?H=E4rdin?= To: FFmpeg development discussions and patches Date: Tue, 03 Sep 2019 19:29:53 +0200 In-Reply-To: <20190903115359.GS3219@michaelspb> References: <153c1256f94ff72829f35fb0de84fcec5e405d94.camel@acc.umu.se> <20190903115359.GS3219@michaelspb> User-Agent: Evolution 3.30.5-1.1 MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [PATCH/RFC] libavcodec/cinepak: Separate decoding from parsing X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" tis 2019-09-03 klockan 13:53 +0200 skrev Michael Niedermayer: > On Sun, Sep 01, 2019 at 10:58:27PM +0200, Tomas Härdin wrote: > > libavcodec/cinepak.c | 337 ++++++++++++++++++++++++++++--- > > ------------- > > tests/ref/fate/cvid-palette | 1 > > tests/ref/fate/cvid-partial | 1 > > 3 files changed, 215 insertions(+), 124 deletions(-) > > 197f986bfb297c54ee0f208449d4053fcb948645 0001-libavcodec-cinepak- > > Separate-decoding-from-parsing.patch > > From d4bda2edbecb93fa5c5910b8b91a866210368a95 Mon Sep 17 00:00:00 > > 2001 > > From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= > > Date: Sun, 1 Sep 2019 15:37:13 +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 | 337 +++++++++++++++++++++++--------- > > ---- > > tests/ref/fate/cvid-palette | 1 - > > tests/ref/fate/cvid-partial | 1 - > > 3 files changed, 215 insertions(+), 124 deletions(-) > > this does not seem to apply automatically here Oops, forgot to squash WIP commits into a single patch. Squashed, rebased and tested patch attached. Moved some functions around and used format-patch --patience to make the diff smaller. And again, it's still preliminary as I want feedback before going any further /Tomas From bfc4fb532b340d62eb4cd1db5d4b93a273f95b6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= 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(-) diff --git a/libavcodec/cinepak.c b/libavcodec/cinepak.c index aeb15de0ed..03cbb1777a 100644 --- a/libavcodec/cinepak.c +++ b/libavcodec/cinepak.c @@ -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) diff --git a/tests/ref/fate/cvid-palette b/tests/ref/fate/cvid-palette index eae41618b5..975778bafb 100644 --- a/tests/ref/fate/cvid-palette +++ b/tests/ref/fate/cvid-palette @@ -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 diff --git a/tests/ref/fate/cvid-partial b/tests/ref/fate/cvid-partial index 990939db1c..38be650ed5 100644 --- a/tests/ref/fate/cvid-partial +++ b/tests/ref/fate/cvid-partial @@ -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