On Sat, Aug 10, 2024 at 12:34:16PM -0300, James Almer wrote:
> On 8/9/2024 5:09 PM, Michael Niedermayer wrote:
> > Hi
> > 
> > On Fri, Aug 09, 2024 at 03:56:42AM +0200, Kacper Michajlow wrote:
> > > On Fri, 9 Aug 2024 at 00:06, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > > > 
> > > > 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
> > > 
> > > Clearing those buffers in fuzzers does not alleviate this security
> > > issue, as they may still be uninitialized in production code.
> > 
> > The decoders in production clear the buffers. The fuzzer does not
> > so the issues it shows dont exist in production
> > 
> > look yourself in get_buffer.c
> > 
> >                  pool->pools[i] = av_buffer_pool_init(size[i] + 16 + STRIDE_ALIGN - 1,
> >                                                       CONFIG_MEMORY_POISONING ?
> >                                                          NULL :
> >                                                          av_buffer_allocz);
> > its av_buffer_allocz
> 
> 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.
> 
> I wrote the function you just changed with the intention of finding issues 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.
> 
> 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 buffers
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 bad 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, perfectly
fine with me. What i do not agree to is the attempt to force the already very
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 the 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 data

and no matter how hard we try to fix all decoders so they never leak something
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

[...]

-- 
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