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 E57E34CA13 for ; Mon, 12 Aug 2024 19:02:12 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 64B6B68DA3C; Mon, 12 Aug 2024 22:02:09 +0300 (EEST) Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 185F768D90D for ; Mon, 12 Aug 2024 22:02:02 +0300 (EEST) Received: by mail.gandi.net (Postfix) with ESMTPSA id 65FE720002 for ; Mon, 12 Aug 2024 19:02:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=niedermayer.cc; s=gm1; t=1723489321; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=4vHraYlHKfe/OXWSMN/RSlz5J099QjJAPH3IOxpWlwc=; b=MfUxSO6O1ugdpeXzwBtv6nIF78Xjyz8T4u1fq3Mvh0Wn0/80rwwxNsroJNcH7ITMorJPQn JmJmoO+0ly70T1k4VDEbZ0U5TdJJhJn/BQktrgJSkCzXpnloSrTfldlpF0DyMFfJVsKU7h s/x2O7EXCa84UdWc0puAH9LmqLPWoZ9R9KnmegQyEW/X6JOoWRzvVKPQWYTGagKDQ1tugI jFcmRNyAEtaFYfL+9xgYIviMS+zwyAecBJqn3Djvq+gMxTjjHu9fiKfnLol5T8RZeqkWsq qmgpVxbJU4xqoZRBFkrYPWLh2FAGpWrLhSX9pHcvQ5a1kpsm529p7tvSBZ3Oig== Date: Mon, 12 Aug 2024 21:02:00 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20240812190200.GM4991@pb2> References: <20240806221853.959177-1-michael@niedermayer.cc> <20240806221853.959177-5-michael@niedermayer.cc> <79221741-358b-4c9a-9782-51799f2eb416@gmail.com> <20240808212701.GC4991@pb2> <20240809200904.GD4991@pb2> <0bc7bbda-7cc1-428a-9ee8-6749212d6e27@gmail.com> MIME-Version: 1.0 In-Reply-To: <0bc7bbda-7cc1-428a-9ee8-6749212d6e27@gmail.com> X-GND-Sasl: michael@niedermayer.cc Subject: Re: [FFmpeg-devel] [PATCH 5/6] tools/target_dec_fuzzer: Use av_buffer_allocz() to avoid missing slices to have unpredictable content 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="===============7730850327616562104==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============7730850327616562104== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="oLSijTScCavI0RJH" Content-Disposition: inline --oLSijTScCavI0RJH Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Aug 10, 2024 at 12:34:16PM -0300, James Almer wrote: > On 8/9/2024 5:09 PM, Michael Niedermayer wrote: > > Hi > >=20 > > On Fri, Aug 09, 2024 at 03:56:42AM +0200, Kacper Michajlow wrote: > > > On Fri, 9 Aug 2024 at 00:06, Michael Niedermayer wrote: > > > >=20 > > > > On Thu, Aug 08, 2024 at 02:13:12PM -0300, James Almer wrote: > > [...] > > > > If decoders are fed with uninitialized buffers thats a > > > > security issue because there are thousands if not ten thousands of > > > > pathes if you consider the number of decoders and the number > > > > of ways they can hit errors > > >=20 > > > Clearing those buffers in fuzzers does not alleviate this security > > > issue, as they may still be uninitialized in production code. > >=20 > > The decoders in production clear the buffers. The fuzzer does not > > so the issues it shows dont exist in production > >=20 > > look yourself in get_buffer.c > >=20 > > pool->pools[i] =3D av_buffer_pool_init(size[i] + 16 + = STRIDE_ALIGN - 1, > > CONFIG_MEMORY_POI= SONING ? > > NULL : > > av_buffer_allo= cz); > > its av_buffer_allocz >=20 > I disagree. That's from avcodec_default_get_buffer2(). What about DR1 > decoders where the caller is using their own avctx.get_buffer2() callback? > Nothing in the documentation says that the buffers must be zeroed. >=20 > I wrote the function you just changed with the intention of finding issue= s a > library user could trigger, which included allocating buffers exactly as = big > as needed (with no extra padding) and not zeroing it, using lavu helpers > like the get_buffer2() documentation states. >=20 > This change here makes half of that moot, and is hiding potential bugs in > the form of use of uninitialized memory in our decoders. we have several sanitizers, msan is just one of them outside msan, using uninitialized buffers is only having one effect and that is it makes things less reproducable using uninitialized buffers is a security issue. Its a secuirty issue because many of our decoders pass uninitialized data through on errors. An attacker uploads a file with error and gets a encoded file back, that encoded file now contains what was in the memory of these uninitialized buf= fers An attacker is not supposed to be able to read your memory like that we have 481 DR1 decoders. For the use for uninitialized buffers to be safe you need to have every error path on every of these decoders to clean every= bit of the buffer that was not initialized. This is not how you design secure software Design that needs "every" multiplied by "every" to do a specific thing is b= ad security noone volunteered to make all the decoders handle uninitialized buffers Simply making these issues appear in ossfuzz doesnt fix them IMHO If someone wants to work on uninitialized buffer support and fixes, perfect= ly fine with me. What i do not agree to is the attempt to force the already ve= ry busy people to work on and fix these issues when a simply "memset()" avoids the whole issue Again, on one hand one memset() on the other 481 DR1 decoders that clear th= e right bits of the buffer on EVERY error path. Thats like strlcpy() vs strcpy() with no bugs on any use. We know which of = this is a bad idea. Why is it here something we argue about ? because DR1 doesnt document that the buffer contents can leak through (which really is what it should say not "you must clear it") Its good enough if the user app ensures the buffer contains no sensitive da= ta and no matter how hard we try to fix all decoders so they never leak someth= ing thorugh. we should still say the custom buffers should not contain sensitive data, so iam not sure but i dont think we disagree here or do we ? thx [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I do not agree with what you have to say, but I'll defend to the death your right to say it. -- Voltaire --oLSijTScCavI0RJH Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEKAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCZrpcJAAKCRBhHseHBAsP qzAUAJ4//L8aWzf8De1K3ulIQxstkRg60wCcDjRKA3kzMYSfmh/2YmE/MSGoV+0= =v2WU -----END PGP SIGNATURE----- --oLSijTScCavI0RJH-- --===============7730850327616562104== 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". --===============7730850327616562104==--