From patchwork Tue Oct 25 23:59:59 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Cadhalpun X-Patchwork-Id: 1175 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.140.133 with SMTP id o127csp212704vsd; Tue, 25 Oct 2016 17:00:11 -0700 (PDT) X-Received: by 10.28.103.7 with SMTP id b7mr6085731wmc.48.1477440011096; Tue, 25 Oct 2016 17:00:11 -0700 (PDT) Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id bp10si25438888wjc.237.2016.10.25.17.00.10; Tue, 25 Oct 2016 17:00:11 -0700 (PDT) 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; dkim=neutral (body hash did not verify) header.i=@googlemail.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; dmarc=fail (p=QUARANTINE dis=NONE) header.from=googlemail.com Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 50559689D7A; Wed, 26 Oct 2016 03:00:05 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wm0-f46.google.com (mail-wm0-f46.google.com [74.125.82.46]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 18194689D59 for ; Wed, 26 Oct 2016 02:59:58 +0300 (EEST) Received: by mail-wm0-f46.google.com with SMTP id c78so193094675wme.0 for ; Tue, 25 Oct 2016 17:00:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=20120113; h=from:subject:to:references:message-id:date:user-agent:mime-version :in-reply-to; bh=bTSzeCtyVX3Q6d13MJzvMMAc3ZlETrsWYHAzl9/Kyys=; b=rnAs/gkUxhZF4nZpQVZMCsnO3garlzaQN8YfNR8oi6a1BMoqh31taDGAk23WMr0Fsx 7cOcSCShw76lfo5tyReIJjrURWZ0ytp3pk6eGg1mh/h5TT+jFJnyDjuD1z6mlk5v3pj7 E6FUN0fupVVBIuW54Z5bP0LlDGQCtsGOSgGUDszP7bji3aY3JX0blfELpA3JnK0CAGI7 CoX7EV8qd6lDkms4Y0swuZK+p+fRabJ1yV3wiCLGv9FgrXyyc6y2mr8mmlFqq5wfCGA9 GFfPN5Oq4uu2R/Z0qWo2EOGKLeDlv7QCH6JWWe9ER961eiIkhNsJbHeabGM9SQaJnrvr skyQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:subject:to:references:message-id:date :user-agent:mime-version:in-reply-to; bh=bTSzeCtyVX3Q6d13MJzvMMAc3ZlETrsWYHAzl9/Kyys=; b=mmtz+Q+R9VmsznrEpA7yhaK+P7VHhx0u+EO6/mBiZYwOgZSaDUF0eGrl4nx9A8KlLy Zc7vVHeMvgGev/YPYoQ6/I8BF2z9my9yigHK4gC0S5Ll9q434cvkk7Lq2UYvV3fyDUDj kloxEx/gROyTVL3Sn5r7hRe7N02WUeIakJ8G2LmUZ8LT47V/WV+/0qF5Ha8DoPJ1iZS5 E1uebrBFLFd/dbdrwVwTvHSfji7J3/H3xSNp8HOxUkUHYLL7iX/lEQv4Td3sAdcU6urC LwKKD1kpSluprOJb1sgQ099zv0f0XeYowUPL2nF1V/npGYOUB4fzZDvc1QKKBeGbnWIU 2dCg== X-Gm-Message-State: ABUngveWfwSBl1tS0+B+Tf0HNFtY3+XkgdNz6bce2qDIKVwj7wW6vr1EZvVkn9HM9aTXKQ== X-Received: by 10.28.32.132 with SMTP id g126mr3957339wmg.131.1477440001465; Tue, 25 Oct 2016 17:00:01 -0700 (PDT) Received: from [192.168.2.21] (p5B095228.dip0.t-ipconnect.de. [91.9.82.40]) by smtp.googlemail.com with ESMTPSA id uq6sm27680869wjc.37.2016.10.25.17.00.00 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 25 Oct 2016 17:00:00 -0700 (PDT) From: Andreas Cadhalpun X-Google-Original-From: Andreas Cadhalpun To: ffmpeg-devel@ffmpeg.org References: <20161025143938.513e0f94@debian> Message-ID: <4c67b844-070a-fbbc-5f99-4bf7c8e510d7@googlemail.com> Date: Wed, 26 Oct 2016 01:59:59 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.4.0 MIME-Version: 1.0 In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH] avcodec: validate codec parameters in avcodec_parameters_to_context 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" On 25.10.2016 14:56, Hendrik Leppkes wrote: > On Tue, Oct 25, 2016 at 2:39 PM, wm4 wrote: >> On Tue, 25 Oct 2016 09:47:29 +0200 >> Hendrik Leppkes wrote: >> >>> On Tue, Oct 25, 2016 at 1:50 AM, Andreas Cadhalpun >>> wrote: >>>> This should reduce the impact of a demuxer (or API user) setting bogus >>>> codec parameters. >>>> >>>> >>> >>> This seems rather noisy I've added a macro to make it less noisy. >>> and doesn't really solve anything, does it? As you analyze below, it was fixing only a part of the problem. >>> Decoders still need to validate values instead of blindly trusting >>> them, and this just hides some problems in these decoders, Yes, the problem hiding is bad, which is why I added av_assert2's so that developers can easily check if the validation fails. >>> instead of >>> fixing them there. API users of avcodec would not fill >>> AVCodecParameters, they would fill a codec context directly. >> >> You could also argue that the demuxer shouldn't return invalid >> parameters either. > > It should not, but this patch does not address this. Indeed, the demuxers should be fixed in addition to this patch. > There is various combinations of component usage that are possible, > and in fact are used in the real world: > > avformat -> avcodec > other demuxer -> avcodec > avformat -> other decoder > > This patch only addresses the first case, and only if you actually use > this function (which I for example do not, since I have an abstraction > layer in between, so I never have AVCodecParameters and AVCodecContext > in the same function). > So in short, it just doesn't fix much, and you can still get invalid > output from avformat, and potentially still undefined behavior in > avcodec if its fed those values through other means. For the third case, the demuxers have to be fixed. Having the asserts in the central code makes it much easier to find these problems. >> How about this: always convert the params to a temporary codecpar, and >> provide a function to determine the validity of a codecpar. This way >> the check could be done in multiple places without duplicating the code >> needed for it. > > That still sounds odd, although slightly better. At the very least it > should be a dedicated function that checks the values in a key place, > say you want to check params that are fed to a decoder, then call this > check in avcodec_open, because thats something everyone has to call to > use avcodec. I tried to implement the suggestions of both of you, see attached patch. Note that the added asserts are triggered by *many* of my fuzzed samples. I'm happy to write patches for these cases, if we achieve agreement that the central check alone is insufficient. Another noteworthy point is that this patch makes it easy to trigger the av_assert0 in iff_read_packet. I think that assert should be replaced with an error return. Best regards, Andreas From f371be7a027da3958e221b4dc88ad558c1489107 Mon Sep 17 00:00:00 2001 From: Andreas Cadhalpun Date: Tue, 25 Oct 2016 01:45:27 +0200 Subject: [PATCH] avcodec: validate codec parameters This should reduce the impact of a broken demuxer (or API user) setting bogus codec parameters. The av_assert2 calls should ease detecting broken demuxers. Suggested-by: wm4 Signed-off-by: Andreas Cadhalpun --- libavcodec/utils.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 80 insertions(+), 2 deletions(-) diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 87de15f..9773b0b 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -1243,6 +1243,7 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code int ret = 0; AVDictionary *tmp = NULL; const AVPixFmtDescriptor *pixdesc; + AVCodecParameters *par = NULL; if (avcodec_is_open(avctx)) return 0; @@ -1259,8 +1260,14 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code if (!codec) codec = avctx->codec; - if (avctx->extradata_size < 0 || avctx->extradata_size >= FF_MAX_EXTRADATA_SIZE) - return AVERROR(EINVAL); + par = avcodec_parameters_alloc(); + if (!par) + return AVERROR(ENOMEM); + /* check if the codec parameters in the context are valid */ + ret = avcodec_parameters_from_context(par, avctx); + avcodec_parameters_free(&par); + if (ret < 0) + return ret; if (options) av_dict_copy(&tmp, *options, 0); @@ -4124,6 +4131,66 @@ static void codec_parameters_reset(AVCodecParameters *par) par->level = FF_LEVEL_UNKNOWN; } +#define CHECK_PARAMETER(parameter, name, check) { \ + av_assert2(!(check)); \ + if (check) { \ + av_log(avctx, AV_LOG_ERROR, "Invalid " name " %d\n", par->parameter); \ + return AVERROR(EINVAL); \ + } \ +} + +static int codec_parameters_check(const AVCodecContext *codec, const AVCodecParameters *par) +{ + AVCodecContext *avctx = (AVCodecContext *)codec; + if (par->bit_rate < 0) { + av_log(avctx, AV_LOG_ERROR, "Invalid bit rate %"PRId64"\n", par->bit_rate); + av_assert2(0); + return AVERROR(EINVAL); + } + CHECK_PARAMETER(bits_per_coded_sample, "bits per coded sample", par->bits_per_coded_sample < 0) + CHECK_PARAMETER(bits_per_raw_sample, "bits per raw sample", par->bits_per_raw_sample < 0) + CHECK_PARAMETER(extradata_size, "extradata size", par->extradata_size < 0 || par->extradata_size >= FF_MAX_EXTRADATA_SIZE) + + switch (par->codec_type) { + case AVMEDIA_TYPE_VIDEO: + if ( (par->width || par->height) && av_image_check_size(par->width, par->height, 0, avctx) < 0) { + av_assert2(0); + return AVERROR(EINVAL); + } + CHECK_PARAMETER(color_range, "color range", (unsigned)par->color_range > AVCOL_RANGE_NB) + CHECK_PARAMETER(color_primaries, "color primaries", (unsigned)par->color_primaries > AVCOL_PRI_NB) + CHECK_PARAMETER(color_trc, "color transfer characteristics ", (unsigned)par->color_trc > AVCOL_PRI_NB) + CHECK_PARAMETER(color_space, "color space", (unsigned)par->color_space > AVCOL_SPC_NB) + CHECK_PARAMETER(chroma_location, "chroma location", (unsigned)par->chroma_location > AVCHROMA_LOC_NB) + if (par->sample_aspect_ratio.num < 0 || par->sample_aspect_ratio.den < 0) { + av_log(avctx, AV_LOG_ERROR, "Invalid sample aspect ratio %d/%d\n", + par->sample_aspect_ratio.num, par->sample_aspect_ratio.den); + av_assert2(0); + return AVERROR(EINVAL); + } + CHECK_PARAMETER(video_delay, "video delay", par->video_delay < 0) + break; + case AVMEDIA_TYPE_AUDIO: + CHECK_PARAMETER(format, "sample format", par->format < -1 || par->format > AV_SAMPLE_FMT_NB) + CHECK_PARAMETER(channels, "number of channels", par->channels < 0) + CHECK_PARAMETER(sample_rate, "sample rate", par->sample_rate < 0) + CHECK_PARAMETER(block_align, "block alignment", par->block_align < 0) + CHECK_PARAMETER(frame_size, "frame size", par->frame_size < 0) + CHECK_PARAMETER(initial_padding, "initial padding", par->initial_padding < 0) + CHECK_PARAMETER(trailing_padding, "trailing padding", par->trailing_padding < 0) + CHECK_PARAMETER(seek_preroll, "seek preroll", par->seek_preroll < 0) + break; + case AVMEDIA_TYPE_SUBTITLE: + if ((par->width || par->height) && av_image_check_size(par->width, par->height, 0, avctx) < 0) { + av_assert2(0); + return AVERROR(EINVAL); + } + break; + } + return 0; +} + + AVCodecParameters *avcodec_parameters_alloc(void) { AVCodecParameters *par = av_mallocz(sizeof(*par)); @@ -4166,6 +4233,7 @@ int avcodec_parameters_copy(AVCodecParameters *dst, const AVCodecParameters *src int avcodec_parameters_from_context(AVCodecParameters *par, const AVCodecContext *codec) { + int ret; codec_parameters_reset(par); par->codec_type = codec->codec_type; @@ -4217,12 +4285,22 @@ int avcodec_parameters_from_context(AVCodecParameters *par, par->extradata_size = codec->extradata_size; } + ret = codec_parameters_check(codec, par); + if (ret < 0) { + codec_parameters_reset(par); + return ret; + } + return 0; } int avcodec_parameters_to_context(AVCodecContext *codec, const AVCodecParameters *par) { + int ret = codec_parameters_check(codec, par); + if (ret < 0) + return ret; + codec->codec_type = par->codec_type; codec->codec_id = par->codec_id; codec->codec_tag = par->codec_tag; -- 2.9.3