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 <mich...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
