From patchwork Thu Nov 17 18:35:01 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Cadhalpun X-Patchwork-Id: 1457 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.90.1 with SMTP id o1csp928187vsb; Thu, 17 Nov 2016 10:35:14 -0800 (PST) X-Received: by 10.195.18.71 with SMTP id gk7mr3675497wjd.175.1479407714638; Thu, 17 Nov 2016 10:35:14 -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 ye10si4003020wjb.105.2016.11.17.10.35.14; Thu, 17 Nov 2016 10:35:14 -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 C165C689F95; Thu, 17 Nov 2016 20:35:09 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wm0-f54.google.com (mail-wm0-f54.google.com [74.125.82.54]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 4260E689F7F for ; Thu, 17 Nov 2016 20:35:03 +0200 (EET) Received: by mail-wm0-f54.google.com with SMTP id t79so163389591wmt.0 for ; Thu, 17 Nov 2016 10:35:04 -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=clHeBGAxdYhJfNbD2BNpTpDNba87wLXw55Naok7+3vA=; b=OZe1PAhytqKaja25q9YPzhn88BRukmgNuIJxyyPt4hspzhx7kodufwtn9knG6ihnnz 3A54S/nSb7A/oY1kfSuqeElgh6AFzz7/mkyr+dt9yP0xBeMrNWg6LUGDFFRIvSftiMWM eEQegMAm3eQV65wmHLzhgXeJkPOdaqRtlB1Ihn4Fcat5ukP1IY/+zkG2r60LMtT9jSvU aBGjARsfdcRi0cDCvpqGxmDN1SA1jU/0MGj2gkUGGs45y+DbNs//G3ieaZsl7afpJg0L /ky/+QAAec6awtSNhdhgXZlIGBMjl7KXejamms9TWPIp91dV2BJdxVEzskd6+WH48Knr 4jag== 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=clHeBGAxdYhJfNbD2BNpTpDNba87wLXw55Naok7+3vA=; b=eyPK9iK1E3rv+2M+fzKyFzSQw2pj+o4c5KQJp9KgdJXUVJxiOC2Jq//1TfRsv7odzb PsgJUJ7Gs6jHAaqTQJKZpYB4l1Dq7WwfgHKZJ0EXsjDWjUGIzf5ms5MeyZKBeivEub71 +7FIu7ERS9godoKjvEvvRaQWykH8MVczAYG72AqNll6DlKH3QXgLyRbBZ1l8vRtBmUFS 4wDQQz2uWjb2YnKJeOX3H7fvKSuXhUgCUxDctqp+LC+qEpt4xOfC+F/11p7fg4Nh1BFs UVnDii13XaI8UWaJJ25jBt5QL7Wvb77T/VBikcDFMQNQgfhq+I9KUqV7xNCTWv+5kvVy xaqg== X-Gm-Message-State: AKaTC01WP3Wju+lArMuKQlDrWwPfotsxw5u3DhldlDw4qFoEUAAeV474j8jCjVChKqs4lA== X-Received: by 10.194.236.135 with SMTP id uu7mr3137967wjc.195.1479407703741; Thu, 17 Nov 2016 10:35:03 -0800 (PST) Received: from [192.168.2.21] (pD9E8F8EC.dip0.t-ipconnect.de. [217.232.248.236]) by smtp.googlemail.com with ESMTPSA id yg1sm4638805wjb.12.2016.11.17.10.35.02 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 17 Nov 2016 10:35:03 -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> Message-ID: <8cef9df2-9cc2-663f-bdbe-21484c1d1823@googlemail.com> Date: Thu, 17 Nov 2016 19:35:01 +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: <20161117012630.GP4824@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 17.11.2016 02:26, Michael Niedermayer wrote: > On Thu, Nov 17, 2016 at 01:08:31AM +0100, Andreas Cadhalpun wrote: >> + SANITIZE_PARAMETER(pix_fmt, "pixel format", codec->pix_fmt < AV_PIX_FMT_NONE || codec->pix_fmt > AV_PIX_FMT_NB, AV_PIX_FMT_NONE) >> + SANITIZE_PARAMETER(bits_per_coded_sample, "bits per coded sample", codec->bits_per_coded_sample < 0, 0) >> + SANITIZE_PARAMETER(bits_per_raw_sample, "bits per raw sample", codec->bits_per_raw_sample < 0, 0) >> + SANITIZE_PARAMETER(extradata_size, "extradata size", codec->extradata_size < 0 || codec->extradata_size >= FF_MAX_EXTRADATA_SIZE, 0) >> + SANITIZE_PARAMETER(color_range, "color range", (unsigned)codec->color_range > AVCOL_RANGE_NB, AVCOL_RANGE_UNSPECIFIED) >> + SANITIZE_PARAMETER(color_primaries, "color primaries", (unsigned)codec->color_primaries > AVCOL_PRI_NB, AVCOL_PRI_UNSPECIFIED) >> + SANITIZE_PARAMETER(color_trc, "color transfer characteristics ", (unsigned)codec->color_trc > AVCOL_TRC_NB, AVCOL_TRC_UNSPECIFIED) >> + SANITIZE_PARAMETER(colorspace, "color space", (unsigned)codec->colorspace > AVCOL_SPC_NB, AVCOL_SPC_UNSPECIFIED) >> + SANITIZE_PARAMETER(chroma_sample_location, "chroma location", (unsigned)codec->chroma_sample_location > AVCHROMA_LOC_NB, AVCHROMA_LOC_UNSPECIFIED) >> + SANITIZE_PARAMETER(has_b_frames, "video delay", codec->has_b_frames < 0, 0) >> + SANITIZE_PARAMETER(sample_fmt, "sample format", codec->sample_fmt < AV_SAMPLE_FMT_NONE || codec->sample_fmt > AV_SAMPLE_FMT_NB, AV_SAMPLE_FMT_NONE) > > This breaks API/ABI You mean this uses private API/ABI. > for example AVCOL_SPC_NB is not part of the public API of libavutil But it's already used in e.g. libavcodec/options_table.h -- which reminds me that this is a much better place to sanitize options. I'll send a separate patch doing that. Attached is an updated version of this patch. > one should be able to use av_color_space_name() to detect invalid color > spaces Indeed, that would have worked, too. Best regards, Andreas From 9bc15dfba44533c9e0f2cc54eec519b359f7f0c5 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 | 105 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 62 insertions(+), 43 deletions(-) diff --git a/libavformat/ffmdec.c b/libavformat/ffmdec.c index 6b09be2..d5e10b0 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,29 @@ 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_WARNING, "Invalid extradata size %d\n", size); + } 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 +387,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 +447,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 +576,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 +594,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 +649,26 @@ 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_WARNING, "Invalid extradata size %d\n", size); + } 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