From patchwork Sat Nov 19 13:04:44 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Cadhalpun X-Patchwork-Id: 1484 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.90.1 with SMTP id o1csp625635vsb; Sat, 19 Nov 2016 05:04:56 -0800 (PST) X-Received: by 10.28.193.199 with SMTP id r190mr3191170wmf.58.1479560696188; Sat, 19 Nov 2016 05:04:56 -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 gr7si11762530wjb.113.2016.11.19.05.04.55; Sat, 19 Nov 2016 05:04:56 -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; 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 13553689DD5; Sat, 19 Nov 2016 15:04:52 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wm0-f50.google.com (mail-wm0-f50.google.com [74.125.82.50]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id C6CB5689B0E for ; Sat, 19 Nov 2016 15:04:44 +0200 (EET) Received: by mail-wm0-f50.google.com with SMTP id f82so76062023wmf.1 for ; Sat, 19 Nov 2016 05:04:46 -0800 (PST) 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=CDAs8T9HjvZJ7WzQzeshZkUdptNiJJ0IE/7KGe0l5ko=; b=p3EO/4XPGJVC3PRhU2knAY2j9wlhI39evlT6NHqrbv+TswJagEayoyP0PWtynpAGSZ 7gm5X5wEUt3sgYg4TGew7/4I8HLTGs+u6AcS6DvyOzA25VUrre2vZOvceDYBHsX7l3Wo P+D4bjzssNFVSSqxhlF2QtEU+5us5DKwJ6Ue56FS4p8u0+BGJOKHhBqAuKrfLsarILaD qaLRA0TXZQxeMXeuhAWklENEHW1P7/lKTx9A0HCOgdUUUojRNVHko3UkeNDfQzHgAhWI CdVKp+ZPJl0CWt8w3wvJmDCvurBOMO4XqlwxGhuGmREyvbPKiWQl41zWuyIj5aa8guB/ 5sJA== 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=CDAs8T9HjvZJ7WzQzeshZkUdptNiJJ0IE/7KGe0l5ko=; b=Cj9Pu1g1qyPMCHeau6CK3hxSjGsPQlO9akn3/UycS4VDzG4RBEU1yDcDe9q6nV7eX4 Llww4Xk5IDPImdtcjLQddG+N+uJ/xrztNRyajK2ck+MKlppLpab7bIxO0mTBDniigqiA UMuAVe9J0YOj7btUZhj+5Vq8p+rkggWsk6n9rx/aufb9ZqScBKwJ+l1FEvPp4DtnuA1g hDYG1fnIHw+H93FhmTSHqtMw82iI575kY7hW/PdZ33EHiqQKlonN2sSwaIJP1TwpUyxS PLPtHOQCQfHFWDm5sn/TVnGuJ5Y2cGBDQYlO+VixSeDll+liCNENScBYraJXwS0t58YJ RGTA== X-Gm-Message-State: AKaTC02DipKPqph3M+m+LpAK+YHbL6XDmMZKkI8c1gh+8IyFgY4w1cakJwSVw2O+NQse/g== X-Received: by 10.28.52.76 with SMTP id b73mr3611959wma.8.1479560686131; Sat, 19 Nov 2016 05:04:46 -0800 (PST) Received: from [192.168.2.21] (p5B095CF1.dip0.t-ipconnect.de. [91.9.92.241]) by smtp.googlemail.com with ESMTPSA id d65sm8678459wmh.11.2016.11.19.05.04.45 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 19 Nov 2016 05:04:45 -0800 (PST) From: Andreas Cadhalpun X-Google-Original-From: Andreas Cadhalpun To: ffmpeg-devel@ffmpeg.org References: <6d786760-6b84-d460-8928-d2a5331d8a60@googlemail.com> <20161117012630.GP4824@nb4> <8cef9df2-9cc2-663f-bdbe-21484c1d1823@googlemail.com> <20161118014057.GX4824@nb4> <6ad11546-7279-977b-097d-49e84f7a14f5@googlemail.com> <20161119011433.GM4824@nb4> Message-ID: <54f9a7a9-4917-86aa-bd91-4872552e8282@googlemail.com> Date: Sat, 19 Nov 2016 14:04:44 +0100 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: <20161119011433.GM4824@nb4> Subject: Re: [FFmpeg-devel] [PATCH] ffmdec: sanitize codec parameters 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 19.11.2016 02:14, Michael Niedermayer wrote: > On Fri, Nov 18, 2016 at 10:35:29PM +0100, Andreas Cadhalpun wrote: >> On 18.11.2016 02:40, Michael Niedermayer wrote: >>> On Thu, Nov 17, 2016 at 07:35:01PM +0100, Andreas Cadhalpun wrote: >>>> + if (size < 0 || size >= FF_MAX_EXTRADATA_SIZE) { >>>> + av_log(s, AV_LOG_WARNING, "Invalid extradata size %d\n", size); >>> >>> i think this and possibly others should be a hard failure >>> or why would a invalid extradata_size be ok ? >> >> The decoder might still be able to work. >> What would be the advantage of a hard failure? > > the advantage of stoping clearly invalid data early > The decoder runs on a seperate machine with ffm possibly. The file > just gets demuxed and sent over the network. The warning hinting to > the issue is on the sending side. The decoder failure at the receiver > i didnt try but if iam not mixing things up that would be confusing > neither side would fully understand whats going on without the other OK, I changed the extradata_size case to an error. Which others do you think should be changed, too? > Does this corrupted but working data occur in practice? I don't know, but hopefully data doesn't get corrupted too often. Best regards, Andreas From 6ef854846a12fe054b81f36ff7ba50b82c4bca34 Mon Sep 17 00:00:00 2001 From: Andreas Cadhalpun Date: Thu, 17 Nov 2016 00:04:57 +0100 Subject: [PATCH] ffmdec: sanitize codec parameters A negative extradata size for example gets passed to memcpy in avcodec_parameters_from_context causing a segmentation fault. Signed-off-by: Andreas Cadhalpun --- libavformat/ffmdec.c | 107 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 64 insertions(+), 43 deletions(-) diff --git a/libavformat/ffmdec.c b/libavformat/ffmdec.c index 6b09be2..d8d5e36 100644 --- a/libavformat/ffmdec.c +++ b/libavformat/ffmdec.c @@ -21,6 +21,7 @@ #include +#include "libavutil/imgutils.h" #include "libavutil/internal.h" #include "libavutil/intreadwrite.h" #include "libavutil/intfloat.h" @@ -28,6 +29,7 @@ #include "libavutil/avassert.h" #include "libavutil/avstring.h" #include "libavutil/pixdesc.h" +#include "libavcodec/internal.h" #include "avformat.h" #include "internal.h" #include "ffm.h" @@ -277,6 +279,13 @@ static int ffm_append_recommended_configuration(AVStream *st, char **conf) return 0; } +#define SANITIZE_PARAMETER(parameter, name, check, default) { \ + if (check) { \ + av_log(codec, AV_LOG_WARNING, "Invalid " name " %d\n", codec->parameter); \ + codec->parameter = default; \ + } \ +} + static int ffm2_read_header(AVFormatContext *s) { FFMContext *ffm = s->priv_data; @@ -346,23 +355,30 @@ static int ffm2_read_header(AVFormatContext *s) } codec->codec_type = avio_r8(pb); if (codec->codec_type != codec_desc->type) { - av_log(s, AV_LOG_ERROR, "Codec type mismatch: expected %d, found %d\n", + av_log(s, AV_LOG_WARNING, "Codec type mismatch: expected %d, found %d\n", codec_desc->type, codec->codec_type); - codec->codec_id = AV_CODEC_ID_NONE; - codec->codec_type = AVMEDIA_TYPE_UNKNOWN; - goto fail; + codec->codec_type = codec_desc->type; } codec->bit_rate = avio_rb32(pb); + if (codec->bit_rate < 0) { + av_log(codec, AV_LOG_WARNING, "Invalid bit rate %"PRId64"\n", codec->bit_rate); + codec->bit_rate = 0; + } codec->flags = avio_rb32(pb); codec->flags2 = avio_rb32(pb); codec->debug = avio_rb32(pb); if (codec->flags & AV_CODEC_FLAG_GLOBAL_HEADER) { int size = avio_rb32(pb); - codec->extradata = av_mallocz(size + AV_INPUT_BUFFER_PADDING_SIZE); - if (!codec->extradata) - return AVERROR(ENOMEM); - codec->extradata_size = size; - avio_read(pb, codec->extradata, size); + if (size < 0 || size >= FF_MAX_EXTRADATA_SIZE) { + av_log(s, AV_LOG_ERROR, "Invalid extradata size %d\n", size); + return AVERROR_INVALIDDATA; + } else { + codec->extradata = av_mallocz(size + AV_INPUT_BUFFER_PADDING_SIZE); + if (!codec->extradata) + return AVERROR(ENOMEM); + codec->extradata_size = size; + avio_read(pb, codec->extradata, size); + } } break; case MKBETAG('S', 'T', 'V', 'I'): @@ -372,21 +388,21 @@ static int ffm2_read_header(AVFormatContext *s) } codec->time_base.num = avio_rb32(pb); codec->time_base.den = avio_rb32(pb); - if (codec->time_base.num <= 0 || codec->time_base.den <= 0) { - av_log(s, AV_LOG_ERROR, "Invalid time base %d/%d\n", + if (codec->time_base.num < 0 || codec->time_base.den <= 0) { + av_log(s, AV_LOG_WARNING, "Invalid time base %d/%d\n", codec->time_base.num, codec->time_base.den); - ret = AVERROR_INVALIDDATA; - goto fail; + codec->time_base.num = 0; + codec->time_base.den = 1; } codec->width = avio_rb16(pb); codec->height = avio_rb16(pb); + if (av_image_check_size(codec->width, codec->height, 0, s) < 0) { + codec->width = 0; + codec->height = 0; + } codec->gop_size = avio_rb16(pb); codec->pix_fmt = avio_rb32(pb); - if (!av_pix_fmt_desc_get(codec->pix_fmt)) { - av_log(s, AV_LOG_ERROR, "Invalid pix fmt id: %d\n", codec->pix_fmt); - codec->pix_fmt = AV_PIX_FMT_NONE; - goto fail; - } + SANITIZE_PARAMETER(pix_fmt, "pixel format", codec->pix_fmt != AV_PIX_FMT_NONE && !av_pix_fmt_desc_get(codec->pix_fmt), AV_PIX_FMT_NONE) codec->qmin = avio_r8(pb); codec->qmax = avio_r8(pb); codec->max_qdiff = avio_r8(pb); @@ -432,13 +448,11 @@ static int ffm2_read_header(AVFormatContext *s) goto fail; } codec->sample_rate = avio_rb32(pb); - if (codec->sample_rate <= 0) { - av_log(s, AV_LOG_ERROR, "Invalid sample rate %d\n", codec->sample_rate); - ret = AVERROR_INVALIDDATA; - goto fail; - } + SANITIZE_PARAMETER(sample_rate, "sample rate", codec->sample_rate < 0, 0) codec->channels = avio_rl16(pb); + SANITIZE_PARAMETER(channels, "number of channels", codec->channels < 0, 0) codec->frame_size = avio_rl16(pb); + SANITIZE_PARAMETER(frame_size, "frame size", codec->frame_size < 0, 0) break; case MKBETAG('C', 'P', 'R', 'V'): if (f_cprv++) { @@ -563,13 +577,15 @@ static int ffm_read_header(AVFormatContext *s) } codec->codec_type = avio_r8(pb); /* codec_type */ if (codec->codec_type != codec_desc->type) { - av_log(s, AV_LOG_ERROR, "Codec type mismatch: expected %d, found %d\n", + av_log(s, AV_LOG_WARNING, "Codec type mismatch: expected %d, found %d\n", codec_desc->type, codec->codec_type); - codec->codec_id = AV_CODEC_ID_NONE; - codec->codec_type = AVMEDIA_TYPE_UNKNOWN; - goto fail; + codec->codec_type = codec_desc->type; } codec->bit_rate = avio_rb32(pb); + if (codec->bit_rate < 0) { + av_log(codec, AV_LOG_WARNING, "Invalid bit rate %"PRId64"\n", codec->bit_rate); + codec->bit_rate = 0; + } codec->flags = avio_rb32(pb); codec->flags2 = avio_rb32(pb); codec->debug = avio_rb32(pb); @@ -579,19 +595,20 @@ static int ffm_read_header(AVFormatContext *s) codec->time_base.num = avio_rb32(pb); codec->time_base.den = avio_rb32(pb); if (codec->time_base.num <= 0 || codec->time_base.den <= 0) { - av_log(s, AV_LOG_ERROR, "Invalid time base %d/%d\n", + av_log(s, AV_LOG_WARNING, "Invalid time base %d/%d\n", codec->time_base.num, codec->time_base.den); - goto fail; + codec->time_base.num = 0; + codec->time_base.den = 1; } codec->width = avio_rb16(pb); codec->height = avio_rb16(pb); + if (av_image_check_size(codec->width, codec->height, 0, s) < 0) { + codec->width = 0; + codec->height = 0; + } codec->gop_size = avio_rb16(pb); codec->pix_fmt = avio_rb32(pb); - if (!av_pix_fmt_desc_get(codec->pix_fmt)) { - av_log(s, AV_LOG_ERROR, "Invalid pix fmt id: %d\n", codec->pix_fmt); - codec->pix_fmt = AV_PIX_FMT_NONE; - goto fail; - } + SANITIZE_PARAMETER(pix_fmt, "pixel format", codec->pix_fmt != AV_PIX_FMT_NONE && !av_pix_fmt_desc_get(codec->pix_fmt), AV_PIX_FMT_NONE) codec->qmin = avio_r8(pb); codec->qmax = avio_r8(pb); codec->max_qdiff = avio_r8(pb); @@ -633,23 +650,27 @@ static int ffm_read_header(AVFormatContext *s) break; case AVMEDIA_TYPE_AUDIO: codec->sample_rate = avio_rb32(pb); - if (codec->sample_rate <= 0) { - av_log(s, AV_LOG_ERROR, "Invalid sample rate %d\n", codec->sample_rate); - goto fail; - } + SANITIZE_PARAMETER(sample_rate, "sample rate", codec->sample_rate < 0, 0) codec->channels = avio_rl16(pb); + SANITIZE_PARAMETER(channels, "number of channels", codec->channels < 0, 0) codec->frame_size = avio_rl16(pb); + SANITIZE_PARAMETER(frame_size, "frame size", codec->frame_size < 0, 0) break; default: goto fail; } if (codec->flags & AV_CODEC_FLAG_GLOBAL_HEADER) { int size = avio_rb32(pb); - codec->extradata = av_mallocz(size + AV_INPUT_BUFFER_PADDING_SIZE); - if (!codec->extradata) - return AVERROR(ENOMEM); - codec->extradata_size = size; - avio_read(pb, codec->extradata, size); + if (size < 0 || size >= FF_MAX_EXTRADATA_SIZE) { + av_log(s, AV_LOG_ERROR, "Invalid extradata size %d\n", size); + return AVERROR_INVALIDDATA; + } else { + codec->extradata = av_mallocz(size + AV_INPUT_BUFFER_PADDING_SIZE); + if (!codec->extradata) + return AVERROR(ENOMEM); + codec->extradata_size = size; + avio_read(pb, codec->extradata, size); + } } avcodec_parameters_from_context(st->codecpar, codec); -- 2.10.2