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 638FA493B8 for ; Fri, 9 Aug 2024 01:57:08 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id E4D4768DA30; Fri, 9 Aug 2024 04:57:05 +0300 (EEST) Received: from mail-ej1-f52.google.com (mail-ej1-f52.google.com [209.85.218.52]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 4D87068D9B8 for ; Fri, 9 Aug 2024 04:56:59 +0300 (EEST) Received: by mail-ej1-f52.google.com with SMTP id a640c23a62f3a-a77ec5d3b0dso178230966b.0 for ; Thu, 08 Aug 2024 18:56:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1723168618; x=1723773418; darn=ffmpeg.org; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :from:to:cc:subject:date:message-id:reply-to; bh=54kRaIKQZSaGiVk01xYxWkAX+KQ91gN4kzkpOpbmot0=; b=Bs8atIOn6SES+r3LrWbSpczzaKr0nXBeOqORPrC8l4yjDzSWWgjlWJSKXFol7cMgLh bL5R9df/hr357uCSKgy/Jr2seVjgddZw5UPjQ35oau/yGtFekf0Laror3wbPyJeUKqpJ 5IC6USAltLWIIyfa1XF9UFPYd7TBt3gS8K+nFz59pJPRlgKiSMBpFLDoUi4LmopYSss6 aWiI702c+yVb0b7c7yg8oTXAJLmLhmwASAc7aaUb75xGbvjFfunZJ6pJeCegZ+mQE4eO SiiqGd8vA4qE9VHjb2EZxetZ0VVq9GQAvidmWqBmZ3hDxklwuwU3sKzfzohX6M+bIrs2 Yvww== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723168618; x=1723773418; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=54kRaIKQZSaGiVk01xYxWkAX+KQ91gN4kzkpOpbmot0=; b=Tbf+saHIjI4JYRWNnBOycZGgD8hssIbBWFMK9YEB1F6V/6N8V31IvGv5fpXZGAN+u4 cSrpd/Af496lVcCIzte8c3xggI4moUyQiUPvc3D4rtERx4DrshKXzRykFY1zPomrt6zg 0xszQhHUqHoWO5aZtyqUHo7pzrc8A0u0F3gvskO50wulifiQ6ceP2wATneMjAWouUmvH EDkl/WL4SZfPjdwIslgohb3uCE6iZjCEexrXfUSzMGcC6YnasbmpLPloOmT+lo1/BwQF mXgHLslLVTeRVB+eFiGQrOo6W2P1eo0Wds+Ydyz+aCRRZ8Uf3kwxXjOXMBiQCl064Yu9 BUAA== X-Gm-Message-State: AOJu0YypPx+oQsd+nv5+MsoXmS+tKzr+I9m65kWuE7qCZ5o7gmusTALW bMr/musIiHuII1GlWh27+cT1NoTgfeoUqlRc2RI4AyjtA9L120URzoLrlOgwc0nGtTqlvmYdKUJ GkCxWG6jJBZE5yur1us79SJfNJ+rAygH+nFQ= X-Google-Smtp-Source: AGHT+IEuNAMqb58/+mesbvG88p6CZrz4fYSbHxne4wMniWVUrbTuj2IUqBVu3Pk7+AUMSqPEJ7BHCujAkJig5KCORQk= X-Received: by 2002:a17:907:f19a:b0:a6f:4fc8:266b with SMTP id a640c23a62f3a-a80aa55bf49mr7761666b.3.1723168617986; Thu, 08 Aug 2024 18:56:57 -0700 (PDT) MIME-Version: 1.0 References: <20240806221853.959177-1-michael@niedermayer.cc> <20240806221853.959177-5-michael@niedermayer.cc> <79221741-358b-4c9a-9782-51799f2eb416@gmail.com> <20240808212701.GC4991@pb2> In-Reply-To: <20240808212701.GC4991@pb2> From: Kacper Michajlow Date: Fri, 9 Aug 2024 03:56:42 +0200 Message-ID: To: FFmpeg development discussions and patches 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: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On Fri, 9 Aug 2024 at 00:06, Michael Niedermayer wrote: > > On Thu, Aug 08, 2024 at 02:13:12PM -0300, James Almer wrote: > > On 8/6/2024 7:18 PM, Michael Niedermayer wrote: > > > Fixes: use of uninitialized values > > > Fixes: 70885/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP6F_fuzzer-4610946029387776 (and likely others) > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer > > > --- > > > tools/target_dec_fuzzer.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/tools/target_dec_fuzzer.c b/tools/target_dec_fuzzer.c > > > index d2d7e21dac7..794b5b92cc7 100644 > > > --- a/tools/target_dec_fuzzer.c > > > +++ b/tools/target_dec_fuzzer.c > > > @@ -129,7 +129,7 @@ static int fuzz_video_get_buffer(AVCodecContext *ctx, AVFrame *frame) > > > frame->extended_data = frame->data; > > > for (i = 0; i < 4 && size[i]; i++) { > > > - frame->buf[i] = av_buffer_alloc(size[i]); > > > + frame->buf[i] = av_buffer_allocz(size[i]); > > > if (!frame->buf[i]) > > > goto fail; > > > frame->data[i] = frame->buf[i]->data; > > > > Wouldn't this hide actual decoder bugs too? > > iam not sure i understand what you mean In general, clearing buffers before processing makes MSAN less effective in discovering invalid accesses because they would all appear valid from its point of view. So, I guess the argument was that this could hide actual decoder bugs since the buffers are already initialized by the fuzzing binary itself, which, in theory, is supposed to emulate the worst-case scenario for a tested decoder. > 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 Clearing those buffers in fuzzers does not alleviate this security issue, as they may still be uninitialized in production code. > Pathes in which these buffers are not filled completely, so each > of these pathes would then need to clear the right bits of data. > Basically that means implementing error concealment for every decoder. > AND making sure that error concealment code is 100% bugfree and leaves > never a spot uncleaned and never touched something that was not writen to Isn't that the point of uninitialized access checking? I can't speak to the scale of the problem because I don't know what the issues are. In principle, you don't have to clear each uninitialized path of the buffer that may occur due to an error. Instead, you should ensure that the buffer is not accessed when the error occurs. If decoders rely on external users to provide zeroed buffers to work correctly, then this should be documented as an API requirement. Outputting garbage on errors is acceptable, but if decoders process uninitialized data internally when errors occur, they are, at best, non-deterministic... > Security wise this is not possible for production code, its too > fragile (at least with the number of decoders and active maintainers we have) > (you want less code to have to be bugfree for security not more code having > to be bug free) > > Now this is the fuzzer and not production code, ok. And of course is > great to have error concealment in every decoder > But then this leaves the question, who will do this work? > If noone does it then we will accumulate many msan bugs in ossfuzz that we wont > be able to do much with except ignore them. > This would make the fuzzer less efficient and it would confuse people looking > at the issues MSAN is not forgiving, and I can imagine that stabilizing it could take time. However, suppressing the reports will not make it more efficient. I might not fully understand what you meant, though. That being said, I think the patch makes sense as a short-term solution to suppress the bulk of reports and focus on the remaining ones. However, it would be good to make it clear that, at some point, it should be reverted. As it stands now, no one will remember why it was zeroed out, and it could remain that way indefinitely. Perhaps it should be configurable per decoder. > Or the short punchy reply maybe is > Produce a volunteer who will fix these bugs before declaring them bugs. > And when doing so consider that we have bugfixes on the mailing list for which we > seem to not even have the man power to review and apply them > > so yeah my oppinion is the default should be the simple & easy to maintain way. > If someone declares their decoder to have flawless error concealment (and for some > simple decoders that could be quite simple) these can always be excluded and use > uninitialized buffers in the fuzzer What is the problem with keeping those reports and letting "someone" work on their decoder based on reports? - Kacper _______________________________________________ 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".