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 62004439C3 for ; Mon, 4 Jul 2022 16:55:32 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 1B20668B7D8; Mon, 4 Jul 2022 19:55:30 +0300 (EEST) Received: from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net [217.70.183.198]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 8EB0C68B88C for ; Mon, 4 Jul 2022 19:55:23 +0300 (EEST) Received: (Authenticated sender: michael@niedermayer.cc) by mail.gandi.net (Postfix) with ESMTPSA id E1ED6C0011 for ; Mon, 4 Jul 2022 16:55:22 +0000 (UTC) Date: Mon, 4 Jul 2022 18:55:21 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20220704165521.GK396728@pb2> References: <20220702222532.9609-1-jamrial@gmail.com> <038f9809-c563-6f9b-3640-8587b213c9bb@gmail.com> MIME-Version: 1.0 In-Reply-To: <038f9809-c563-6f9b-3640-8587b213c9bb@gmail.com> 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-Type: multipart/mixed; boundary="===============4274356865244345521==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============4274356865244345521== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="4jP6nrBBjIjUXupb" Content-Disposition: inline --4jP6nrBBjIjUXupb Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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; > >=20 > > > 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. > >=20 > > You are both right. > >=20 > > 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). > >=20 > > What is wrong is Michael's original fix: > >=20 > > > > commit 8154cb7c2ff2afcb1a0842de8c215b7714c814d0 > > > > Author: Michael Niedermayer > > > > Date: 2022-06-30 00:00:32 +0200 > > > >=20 > > > > avutil/channel_layout: av_channel_layout_describe_bprint: Chec= k for buffer end > > > >=20 > > > > Fixes: Timeout printing a billion channels > > > > Fixes: 48099/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSK= A_fuzzer-6754782204788736 > > > >=20 > > > > Found-by: continuous fuzzing process https://github.com/google= /oss-fuzz/tree/master/projects/ffmpeg > > > > Signed-off-by: Michael Niedermayer > > > >=20 > > > > 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 AV= ChannelLayout *channel_layout, > > > > if (channel_layout->order =3D=3D AV_CHANNEL_ORDER_CUS= TOM && > >=20 > > > > channel_layout->u.map[i].name[0]) > >=20 > > If the channel count is insanely high, then this will cause invalid > > reads. > >=20 > > > > av_bprintf(bp, "@%s", channel_layout->u.map[i].na= me); > > > > + > > > > + if (!av_bprint_is_complete(bp)) > > > > + return AVERROR(ENOMEM); > > > > + > > > > } > > > > if (channel_layout->nb_channels) { > > > > av_bprintf(bp, ")"); > >=20 > > 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. >=20 > The Matroska demuxer could in theory generate a native layout with more t= han > 64 channels where popcnt(mask) !=3D channels, and nothing seems to valida= te > what a demuxer's read_header() callback returns if you just call lavf API > functions like target_dem_fuzzer.c seems to do. >=20 > Maybe: >=20 > > 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(AVFormatContex= t *s) > > st->codecpar->codec_tag =3D fourcc; > > st->codecpar->sample_rate =3D track->audio.out_samplerate; > > // channel layout may be already set by codec private chec= ks above > > - if (st->codecpar->ch_layout.order =3D=3D AV_CHANNEL_ORDER_= NATIVE && > > - !st->codecpar->ch_layout.u.mask) > > + if (!av_channel_layout_check(&st->codecpar->ch_layout)) { > > st->codecpar->ch_layout.order =3D AV_CHANNEL_ORDER_UNS= PEC; > > - st->codecpar->ch_layout.nb_channels =3D track->audio.chann= els; > > + st->codecpar->ch_layout.nb_channels =3D track->audio.c= hannels; > > + } > > if (!st->codecpar->bits_per_coded_sample) > > st->codecpar->bits_per_coded_sample =3D track->audio.b= itdepth; > > if (st->codecpar->codec_id =3D=3D AV_CODEC_ID_MP3 || >=20 > 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 ri= ght > now, and it is). >=20 > Assuming i got this right, in this fuzzing sample's case it would still h= ave > a billion channels (since that's what track->audio.channels contains, as > read from the container), but using the unspec layout, which is technical= ly > valid even if nothing will really handle it, and > av_channel_layout_describe() will print a small string. seems this is fixing it thanks >=20 > Still, i think a check in avformat_open_input() might also be a good idea, yes, i agree=20 > especially once (and if) demuxers start propagating custom layouts, where > the map array will be allocated. [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No great genius has ever existed without some touch of madness. -- Aristotle --4jP6nrBBjIjUXupb Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEIAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCYsMbdgAKCRBhHseHBAsP qy02AJ9w8IG9vwt9IVwDbwIrVP6NVYZ/TACdG+QvecMd+2p5dJG2pYJKxBUhW7o= =ufaT -----END PGP SIGNATURE----- --4jP6nrBBjIjUXupb-- --===============4274356865244345521== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ 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". --===============4274356865244345521==--