| Message ID | 20201016104614.12474-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 2be51d14f278c1c207becafed0db20cbc4e89f55 |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avformat/av1dec: Fix padding in obu_get_packet() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
Michael Niedermayer: > Fixes: stack buffer overflow (read) > Fixes: 26369/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-5721057325219840 > Sure this is the right testcase? > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@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; >
On Fri, Oct 16, 2020 at 01:20:15PM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > Fixes: stack buffer overflow (read) > > Fixes: 26369/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-5721057325219840 > > > > Sure this is the right testcase? yes, thats related to the other patch about the fuzer behaving a bit broken thx [...]
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@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.
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@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). - Andreas
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@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.
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@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
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@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
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@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
On Fri, Oct 16, 2020 at 12:46:12PM +0200, 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@niedermayer.cc> > --- > libavformat/av1dec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) will apply [...]
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;
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@niedermayer.cc> --- libavformat/av1dec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)