diff mbox series

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

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()
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Michael Niedermayer Oct. 16, 2020, 10:46 a.m. UTC
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(-)

Comments

Andreas Rheinhardt Oct. 16, 2020, 11:20 a.m. UTC | #1
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;
>
Michael Niedermayer Oct. 16, 2020, 12:58 p.m. UTC | #2
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

[...]
James Almer Oct. 16, 2020, 1:12 p.m. UTC | #3
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.
Andreas Rheinhardt Oct. 16, 2020, 1:23 p.m. UTC | #4
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
James Almer Oct. 16, 2020, 1:30 p.m. UTC | #5
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.
Andreas Rheinhardt Oct. 16, 2020, 1:35 p.m. UTC | #6
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
James Almer Oct. 16, 2020, 1:40 p.m. UTC | #7
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".
>
Andreas Rheinhardt Oct. 16, 2020, 1:54 p.m. UTC | #8
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
Michael Niedermayer Oct. 20, 2020, 1:42 p.m. UTC | #9
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 mbox series

Patch

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;