From patchwork Wed Feb 1 16:25:37 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Steinar H. Gunderson" X-Patchwork-Id: 2403 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.89.21 with SMTP id n21csp2515615vsb; Wed, 1 Feb 2017 08:25:49 -0800 (PST) X-Received: by 10.28.157.195 with SMTP id g186mr1301266wme.6.1485966349310; Wed, 01 Feb 2017 08:25:49 -0800 (PST) Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id y70si21746359wmh.162.2017.02.01.08.25.48; Wed, 01 Feb 2017 08:25:49 -0800 (PST) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id CB6A5680A04; Wed, 1 Feb 2017 18:25:42 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from cassarossa.samfundet.no (cassarossa.samfundet.no [193.35.52.29]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 39D03680974 for ; Wed, 1 Feb 2017 18:25:36 +0200 (EET) Received: from pannekake.samfundet.no ([2001:67c:29f4::50] ident=unknown) by cassarossa.samfundet.no with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from ) id 1cYxiz-00028T-41; Wed, 01 Feb 2017 17:25:38 +0100 Received: from sesse by pannekake.samfundet.no with local (Exim 4.84_2) (envelope-from ) id 1cYxiz-0004Gs-1u; Wed, 01 Feb 2017 17:25:37 +0100 Date: Wed, 1 Feb 2017 17:25:37 +0100 From: "Steinar H. Gunderson" To: Andreas Cadhalpun Message-ID: <20170201162536.GA16014@sesse.net> References: <7b5404a4-fae1-44b0-b97c-6c249ea03280@googlemail.com> <20170130082349.GA10303@sesse.net> <20170130235916.GA2907@sesse.net> <3e6b9afe-8c1a-db9b-3706-4e259f0c9d45@googlemail.com> <20170131084300.GA11652@sesse.net> <938b4720-9a88-9cb3-e624-b782b2f1edf9@googlemail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <938b4720-9a88-9cb3-e624-b782b2f1edf9@googlemail.com> X-Operating-System: Linux 4.9.0 on a x86_64 User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [FFmpeg-devel] [PATCH] speedhq: make sure the block index is not negative 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 Cc: ffmpeg-devel@ffmpeg.org Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" On Wed, Feb 01, 2017 at 02:17:05AM +0100, Andreas Cadhalpun wrote: >> Would you mind sharing an input where this actually triggers? None of the >> samples I have seem to trigger this, so I suppose it's some sort of fuzzed >> input. > Indeed it is. I've sent you a sample. Could you please try the attached patch? /* Steinar */ From d1c914f869edfc4326e86b1b0c161249196e6900 Mon Sep 17 00:00:00 2001 From: "Steinar H. Gunderson" Date: Wed, 1 Feb 2017 17:19:18 +0100 Subject: [PATCH] speedhq: fix out-of-bounds write Certain alpha run lengths (for SHQ1/SHQ3/SHQ5) could be stored in both long and short versions, and we would only accept the short version, returning -1 (invalid code) for the others. This could cause an out-of-bounds write on malicious input, as discovered by Andreas Cadhalpun during fuzzing. Fix by simply allowing both versions, leaving no invalid codes in the alpha VLC. --- libavcodec/speedhq.c | 128 +++++++++++++++++++++++++++++++-------------------- libavcodec/vlc.h | 15 ++++-- 2 files changed, 89 insertions(+), 54 deletions(-) diff --git a/libavcodec/speedhq.c b/libavcodec/speedhq.c index 30160dd3f2..49609342af 100644 --- a/libavcodec/speedhq.c +++ b/libavcodec/speedhq.c @@ -196,7 +196,7 @@ static inline int decode_alpha_block(const SHQContext *s, GetBitContext *gb, uin UPDATE_CACHE_LE(re, gb); GET_VLC(run, re, gb, ff_dc_alpha_run_vlc_le.table, ALPHA_VLC_BITS, 2); - if (run == 128) break; + if (run < 0) break; i += run; if (i >= 128) return AVERROR_INVALIDDATA; @@ -474,61 +474,89 @@ static int speedhq_decode_frame(AVCodecContext *avctx, */ static av_cold void compute_alpha_vlcs(void) { - uint16_t run_code[129], level_code[256]; - uint8_t run_bits[129], level_bits[256]; - int run, level; - - for (run = 0; run < 128; run++) { - if (!run) { - /* 0 -> 0. */ - run_code[run] = 0; - run_bits[run] = 1; - } else if (run <= 4) { - /* 10xx -> xx plus 1. */ - run_code[run] = ((run - 1) << 2) | 1; - run_bits[run] = 4; - } else { - /* 111xxxxxxx -> xxxxxxxx. */ - run_code[run] = (run << 3) | 7; - run_bits[run] = 10; - } + uint16_t run_code[134], level_code[266]; + uint8_t run_bits[134], level_bits[266]; + int16_t run_symbols[134], level_symbols[266]; + int entry, i, sign; + + /* Initialize VLC for alpha run. */ + entry = 0; + + /* 0 -> 0. */ + run_code[entry] = 0; + run_bits[entry] = 1; + run_symbols[entry] = 0; + ++entry; + + /* 10xx -> xx plus 1. */ + for (i = 0; i < 4; ++i) { + run_code[entry] = (i << 2) | 1; + run_bits[entry] = 4; + run_symbols[entry] = i + 1; + ++entry; + } + + /* 111xxxxxxx -> xxxxxxx. */ + for (i = 0; i < 128; ++i) { + run_code[entry] = (i << 3) | 7; + run_bits[entry] = 10; + run_symbols[entry] = i; + ++entry; } /* 110 -> EOB. */ - run_code[128] = 3; - run_bits[128] = 3; - - INIT_LE_VLC_STATIC(&ff_dc_alpha_run_vlc_le, ALPHA_VLC_BITS, 129, - run_bits, 1, 1, - run_code, 2, 2, 160); - - for (level = 0; level < 256; level++) { - int8_t signed_level = (int8_t)level; - int abs_signed_level = abs(signed_level); - int sign = (signed_level < 0) ? 1 : 0; - - if (abs_signed_level == 1) { - /* 1s -> -1 or +1 (depending on sign bit). */ - level_code[level] = (sign << 1) | 1; - level_bits[level] = 2; - } else if (abs_signed_level >= 2 && abs_signed_level <= 5) { - /* 01sxx -> xx plus 2 (2..5 or -2..-5, depending on sign bit). */ - level_code[level] = ((abs_signed_level - 2) << 3) | (sign << 2) | 2; - level_bits[level] = 5; - } else { - /* - * 00xxxxxxxx -> xxxxxxxx, in two's complement. 0 is technically an - * illegal code (that would be encoded by increasing run), but it - * doesn't hurt and simplifies indexing. - */ - level_code[level] = level << 2; - level_bits[level] = 10; + run_code[entry] = 3; + run_bits[entry] = 3; + run_symbols[entry] = -1; + ++entry; + + av_assert0(entry == FF_ARRAY_ELEMS(run_code)); + + INIT_LE_VLC_SPARSE_STATIC(&ff_dc_alpha_run_vlc_le, ALPHA_VLC_BITS, + FF_ARRAY_ELEMS(run_code), + run_bits, 1, 1, + run_code, 2, 2, + run_symbols, 2, 2, 160); + + /* Initialize VLC for alpha level. */ + entry = 0; + + for (sign = 0; sign <= 1; ++sign) { + /* 1s -> -1 or +1 (depending on sign bit). */ + level_code[entry] = (sign << 1) | 1; + level_bits[entry] = 2; + level_symbols[entry] = sign ? -1 : 1; + ++entry; + + /* 01sxx -> xx plus 2 (2..5 or -2..-5, depending on sign bit). */ + for (i = 0; i < 4; ++i) { + level_code[entry] = (i << 3) | (sign << 2) | 2; + level_bits[entry] = 5; + level_symbols[entry] = sign ? -(i + 2) : (i + 2); + ++entry; } } - INIT_LE_VLC_STATIC(&ff_dc_alpha_level_vlc_le, ALPHA_VLC_BITS, 256, - level_bits, 1, 1, - level_code, 2, 2, 288); + /* + * 00xxxxxxxx -> xxxxxxxx, in two's complement. There are many codes + * here that would better be encoded in other ways (e.g. 0 would be + * encoded by increasing run, and +/- 1 would be encoded with a + * shorter code), but it doesn't hurt to allow everything. + */ + for (i = 0; i < 256; ++i) { + level_code[entry] = i << 2; + level_bits[entry] = 10; + level_symbols[entry] = i; + ++entry; + } + + av_assert0(entry == FF_ARRAY_ELEMS(level_code)); + + INIT_LE_VLC_SPARSE_STATIC(&ff_dc_alpha_level_vlc_le, ALPHA_VLC_BITS, + FF_ARRAY_ELEMS(level_code), + level_bits, 1, 1, + level_code, 2, 2, + level_symbols, 2, 2, 288); } static uint32_t reverse(uint32_t num, int bits) diff --git a/libavcodec/vlc.h b/libavcodec/vlc.h index 40096d8944..42ccddf3fc 100644 --- a/libavcodec/vlc.h +++ b/libavcodec/vlc.h @@ -54,21 +54,28 @@ void ff_free_vlc(VLC *vlc); #define INIT_VLC_LE 2 #define INIT_VLC_USE_NEW_STATIC 4 -#define INIT_VLC_STATIC(vlc, bits, a, b, c, d, e, f, g, static_size) \ +#define INIT_VLC_SPARSE_STATIC(vlc, bits, a, b, c, d, e, f, g, h, i, j, static_size) \ do { \ static VLC_TYPE table[static_size][2]; \ (vlc)->table = table; \ (vlc)->table_allocated = static_size; \ - init_vlc(vlc, bits, a, b, c, d, e, f, g, INIT_VLC_USE_NEW_STATIC); \ + ff_init_vlc_sparse(vlc, bits, a, b, c, d, e, f, g, h, i, j, \ + INIT_VLC_USE_NEW_STATIC); \ } while (0) -#define INIT_LE_VLC_STATIC(vlc, bits, a, b, c, d, e, f, g, static_size) \ +#define INIT_LE_VLC_SPARSE_STATIC(vlc, bits, a, b, c, d, e, f, g, h, i, j, static_size) \ do { \ static VLC_TYPE table[static_size][2]; \ (vlc)->table = table; \ (vlc)->table_allocated = static_size; \ - init_vlc(vlc, bits, a, b, c, d, e, f, g, \ + ff_init_vlc_sparse(vlc, bits, a, b, c, d, e, f, g, h, i, j, \ INIT_VLC_USE_NEW_STATIC | INIT_VLC_LE); \ } while (0) +#define INIT_VLC_STATIC(vlc, bits, a, b, c, d, e, f, g, static_size) \ + INIT_VLC_SPARSE_STATIC(vlc, bits, a, b, c, d, e, f, g, NULL, 0, 0, static_size) + +#define INIT_LE_VLC_STATIC(vlc, bits, a, b, c, d, e, f, g, static_size) \ + INIT_LE_VLC_SPARSE_STATIC(vlc, bits, a, b, c, d, e, f, g, NULL, 0, 0, static_size) + #endif /* AVCODEC_VLC_H */ -- 2.11.0