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

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

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
> 
> - Andreas
> _______________________________________________
> 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".
>