[FFmpeg-devel] [PATCH 1/3] avformat/av1dec: Fix padding in obu_get_packet()

[Andreas Rheinhardt]

James Almer:
> On 10/16/2020 10:35 AM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 10/16/2020 10:23 AM, Andreas Rheinhardt wrote:
>>>> James Almer:
>>>>> On 10/16/2020 7:46 AM, Michael Niedermayer wrote:
>>>>>> Fixes: stack buffer overflow (read)
>>>>>> Fixes: 26369/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-5721057325219840
>>>>>>
>>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>>>>> ---
>>>>>>  libavformat/av1dec.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/libavformat/av1dec.c b/libavformat/av1dec.c
>>>>>> index 10c4560968..395eef6522 100644
>>>>>> --- a/libavformat/av1dec.c
>>>>>> +++ b/libavformat/av1dec.c
>>>>>> @@ -382,7 +382,7 @@ static int obu_read_header(AVFormatContext *s)
>>>>>>  static int obu_get_packet(AVFormatContext *s, AVPacket *pkt)
>>>>>>  {
>>>>>>      ObuContext *c = s->priv_data;
>>>>>> -    uint8_t header[MAX_OBU_HEADER_SIZE];
>>>>>> +    uint8_t header[MAX_OBU_HEADER_SIZE + AV_INPUT_BUFFER_PADDING_SIZE];
>>>>>>      int64_t obu_size;
>>>>>>      int size = av_fifo_space(c->fifo);
>>>>>>      int ret, len, type;
>>>>>
>>>>> Where is header being overread? All reads and writes are always
>>>>> constrained to MAX_OBU_HEADER_SIZE bytes at most by the fifo.
>>>>
>>>> read_obu_with_size() reads it via a GetBitContext which overreads (even
>>>> when not using the unchecked bitstream reader).
>>>
>>> I thought about that too, which would mean this fuzzer forcefully
>>> disables the checked bitstream reader at configure time? (Why do we even
>>> have such a configure option anyway? It breaks all kinds of assumptions.
>>> It should be done internally at the module level exclusively).
>>>
>>> Defining UNCHECKED_BITSTREAM_READER to 0 in av1dec.c before including
>>> get_bits.h would be a better fix.
>>
>> You misunderstood: Even the checked bitstream reader overreads
> 
> How useful and expected. It's not like the get_bits.h doxy says the
> checked bitstream reader "ensures that we don't read past input buffer
> boundaries" or anything like that.
> 

"* Initialize GetBitContext.
 * @param buffer bitstream buffer, must be AV_INPUT_BUFFER_PADDING_SIZE
bytes larger than the actual read bits because some optimized bitstream
readers read 32 or 64 bit at once and could read over the end"

(Actually AV_INPUT_BUFFER_PADDING_SIZE is much bigger than 64bit
nowadays. This requirement probably comes from a time when it was
smaller. Maybe we should add a smaller constant?)

> Guess the padding works, then.
> 
>> (otherwise every get_bits() call would need special code to handle the
>> case in which less than four bytes are available). The only difference
>> between the checked and the unchecked bitstream reader is that the
>> former checks when updating the counter:
>>
>> #if UNCHECKED_BITSTREAM_READER
>> #   define SKIP_COUNTER(name, gb, num) name ## _index += (num)
>> #else
>> #   define SKIP_COUNTER(name, gb, num) \
>>     name ## _index = FFMIN(name ## _size_plus8, name ## _index + (num))
>> #endif