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

[Andreas Rheinhardt]

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