From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id C7ADD439C6 for ; Mon, 4 Jul 2022 16:57:09 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 3749368B920; Mon, 4 Jul 2022 19:57:08 +0300 (EEST) Received: from mail-vs1-f53.google.com (mail-vs1-f53.google.com [209.85.217.53]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 93E8D68B86B for ; Mon, 4 Jul 2022 19:57:01 +0300 (EEST) Received: by mail-vs1-f53.google.com with SMTP id 189so9583038vsh.2 for ; Mon, 04 Jul 2022 09:57:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=message-id:date:mime-version:user-agent:subject:content-language:to :references:from:in-reply-to:content-transfer-encoding; bh=pMixFAi7KWg8tk9Lx/UzImL+ZuygywcTIOpdvNoqRMQ=; b=DIEQXXe3VGX1ayE6rKgbn8H39GRwGeYQmMdgwYGCsnO/jmTtukzBKEg5mzqBh2hjMM IBUVhWLu6pNm2YV6mym684GhUrfNgljpdHKX8B4Nhnn+TuZJ5nGnfLeIaosBebuOk5ns XpoiIeslS3vGsi5ky4HKMAZYkZnHp489dGqp2HO/j6A4P+r2EhiiBILK+qGLGhWPboFN no3gb6b0a77fT4XvazCeNm77M0iFhsdLTW8ovj3F6V8FTLdWLquXTlKEncmaHub9Y33T vI1EoDhtPQLt+iMoJ3REkWphNZ++cCOK6dvDhmZYUbiyPCj2PGNCvq0boe1qJVBeOUzj NLBQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:references:from:in-reply-to :content-transfer-encoding; bh=pMixFAi7KWg8tk9Lx/UzImL+ZuygywcTIOpdvNoqRMQ=; b=kvtVj2rqby5Lq0erctxM7KBLnHEtwobAkJ/EhXt6rlTNjQuPk0AoTZwnLVJ2G5tTRz 42qjfw4e5nJ4MlGBJ5/waeQQcDhZMNyO1DzzynnLmQCreNp8bL+IkPDFPg+JmYSL5PoA +m2IOGWZl18fqZUt/WG3jTm5WliSKGocMc89wcjVB7Veh8GWKFzbLP++HPIzsNRs2v0T 56LnmyNdgmyMxkT8PKTcPSx4PU0lW+lDkp/UiTEjepxnul9jffODdbaCJ5Gagku148f6 dV5nTZ18voIMiAgps8hvyCSg5y8cOV2q63K3S2hDAcwRycED97YekqBddDVPtzTZsdDu YdPQ== X-Gm-Message-State: AJIora8DIDR24QZ9QnxocyoPat1x3/dkoHc41Jx4qn9jPZdrxduF2Oel EtfCG3BFYa3BYN//YUNTBkIirh7FLUorqw== X-Google-Smtp-Source: AGRyM1tMxshxx5/Zd+IIkaQYsV1+P7IJ2GGtWuKSBmTzHb7n/ntuJbRMgtxvXjmVMi78v2aX96WNfQ== X-Received: by 2002:a05:6102:1504:b0:354:397f:51c5 with SMTP id f4-20020a056102150400b00354397f51c5mr17645690vsv.63.1656953819382; Mon, 04 Jul 2022 09:56:59 -0700 (PDT) Received: from [192.168.0.13] ([186.136.131.204]) by smtp.gmail.com with ESMTPSA id z63-20020a1fe242000000b00370896b336esm2781506vkg.41.2022.07.04.09.56.58 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 04 Jul 2022 09:56:58 -0700 (PDT) Message-ID: Date: Mon, 4 Jul 2022 13:57:01 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Content-Language: en-US To: ffmpeg-devel@ffmpeg.org References: <20220702222532.9609-1-jamrial@gmail.com> <038f9809-c563-6f9b-3640-8587b213c9bb@gmail.com> <20220704165521.GK396728@pb2> From: James Almer In-Reply-To: <20220704165521.GK396728@pb2> Subject: Re: [FFmpeg-devel] [PATCH] avutil/channel_layout: don't error out on truncated strings X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 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 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On 7/4/2022 1:55 PM, Michael Niedermayer wrote: > On Sun, Jul 03, 2022 at 09:27:31PM -0300, James Almer wrote: >> On 7/3/2022 7:00 AM, Nicolas George wrote: >>> Andreas Rheinhardt (12022-07-03): >>>>> if (!av_bprint_is_complete(bp)) >>>>> - return AVERROR(ENOMEM); >>>>> + break; >>> >>>> Isn't this actually still against the API? av_channel_layout_describe() >>>> will not return the correct number of bytes necessary to write the >>>> string for the channel layout. >>> >>> You are both right. >>> >>> BPrint-based APIs are not supposed to check for truncation, because >>> printing into a bounded buffer to determine the necessary size is a >>> valid use (see AV_BPRINT_SIZE_COUNT_ONLY). >>> >>> What is wrong is Michael's original fix: >>> >>>>> commit 8154cb7c2ff2afcb1a0842de8c215b7714c814d0 >>>>> Author: Michael Niedermayer >>>>> Date: 2022-06-30 00:00:32 +0200 >>>>> >>>>> avutil/channel_layout: av_channel_layout_describe_bprint: Check for buffer end >>>>> >>>>> Fixes: Timeout printing a billion channels >>>>> Fixes: 48099/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-6754782204788736 >>>>> >>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>>>> Signed-off-by: Michael Niedermayer >>>>> >>>>> diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c >>>>> index 21b70173b7..1887987789 100644 >>>>> --- a/libavutil/channel_layout.c >>>>> +++ b/libavutil/channel_layout.c >>>>> @@ -757,6 +757,10 @@ int av_channel_layout_describe_bprint(const AVChannelLayout *channel_layout, >>>>> if (channel_layout->order == AV_CHANNEL_ORDER_CUSTOM && >>> >>>>> channel_layout->u.map[i].name[0]) >>> >>> If the channel count is insanely high, then this will cause invalid >>> reads. >>> >>>>> av_bprintf(bp, "@%s", channel_layout->u.map[i].name); >>>>> + >>>>> + if (!av_bprint_is_complete(bp)) >>>>> + return AVERROR(ENOMEM); >>>>> + >>>>> } >>>>> if (channel_layout->nb_channels) { >>>>> av_bprintf(bp, ")"); >>> >>> Obviously, this fuzzer found a case where a demuxer or a decoder >>> constructs an invalid channel layout in memory without proper >>> validation. There is a bug, possibly an security-related one, and this >>> only hides it from the test suite. >> >> The Matroska demuxer could in theory generate a native layout with more than >> 64 channels where popcnt(mask) != channels, and nothing seems to validate >> what a demuxer's read_header() callback returns if you just call lavf API >> functions like target_dem_fuzzer.c seems to do. >> >> Maybe: >> >>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c >>> index 73ded761fd..ad7ee390a2 100644 >>> --- a/libavformat/matroskadec.c >>> +++ b/libavformat/matroskadec.c >>> @@ -2950,10 +2950,10 @@ static int matroska_parse_tracks(AVFormatContext *s) >>> st->codecpar->codec_tag = fourcc; >>> st->codecpar->sample_rate = track->audio.out_samplerate; >>> // channel layout may be already set by codec private checks above >>> - if (st->codecpar->ch_layout.order == AV_CHANNEL_ORDER_NATIVE && >>> - !st->codecpar->ch_layout.u.mask) >>> + if (!av_channel_layout_check(&st->codecpar->ch_layout)) { >>> st->codecpar->ch_layout.order = AV_CHANNEL_ORDER_UNSPEC; >>> - st->codecpar->ch_layout.nb_channels = track->audio.channels; >>> + st->codecpar->ch_layout.nb_channels = track->audio.channels; >>> + } >>> if (!st->codecpar->bits_per_coded_sample) >>> st->codecpar->bits_per_coded_sample = track->audio.bitdepth; >>> if (st->codecpar->codec_id == AV_CODEC_ID_MP3 || >> >> is enough to ensure a valid layout is propagated. This assumes parameters >> set by codec private parsing are correct (only FLAC seem to be present right >> now, and it is). >> >> Assuming i got this right, in this fuzzing sample's case it would still have >> a billion channels (since that's what track->audio.channels contains, as >> read from the container), but using the unspec layout, which is technically >> valid even if nothing will really handle it, and >> av_channel_layout_describe() will print a small string. > > seems this is fixing it > thanks Ok, will apply it and revert 8154cb7c2f. > > >> >> Still, i think a check in avformat_open_input() might also be a good idea, > > yes, i agree > > >> especially once (and if) demuxers start propagating custom layouts, where >> the map array will be allocated. > > [...] > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".