[FFmpeg-devel] [PATCH 5/6] tools/target_dec_fuzzer: Use av_buffer_allocz() to avoid missing slices to have unpredictable content

[James Almer]

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

> 
> 
> [...]
>>> 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.
> 
> It will make it more efficient because then the fuzzer shows only issues
> also affecting production and ones someone intends to work on
> Otherwise it shows many issues that will distract and confuse
> 
> 
>> I might not fully understand what you meant, though.
> 
> Yes, i think we misunderstand each other a bit
> 
> 
> [...]
> 
>> Perhaps it
>> should be configurable per decoder.
> 
> That is what i suggested, or at least i meant to.
> For decoders where someone intends to fix every case where original buffer
> data with nothing written into it come through it could make sense to enable
> uninitialized input buffers.
> Still i have not seen anyone actually want to do that. I certainly dont have the
> time for any of the decoders that i maintain. But if someone else wants
> i surely dont mind if (s)he turns this on and works on the additional cases for
> any decoders that i maintain ...
> 
> 
>>
>>> 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?
> 
> ossfuzz is the problem,
> these issues are not seperate/segregated nor do i see a way ossfuzz could
> seperate them but again ATM we have noone intending to work on this so
> this patch solves it.
> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".