[FFmpeg-devel] avcodec/av1_parse: Check obu_size

Submitted by Michael Niedermayer on Oct. 14, 2018, 1:43 p.m.

Details

Message ID 20181014134356.31500-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Oct. 14, 2018, 1:43 p.m.
Fixes: out of array read
Fixes: SIGSEGV_get_obu_bit_length_av1_parse

Found-by: keval shah <skeval65@gmail.com>
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/av1_parse.h | 3 +++
 1 file changed, 3 insertions(+)

Comments

James Almer Oct. 14, 2018, 2:03 p.m.
On 10/14/2018 10:43 AM, Michael Niedermayer wrote:
> Fixes: out of array read
> Fixes: SIGSEGV_get_obu_bit_length_av1_parse
> 
> Found-by: keval shah <skeval65@gmail.com>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/av1_parse.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libavcodec/av1_parse.h b/libavcodec/av1_parse.h
> index 276af33ba9..312d8825e1 100644
> --- a/libavcodec/av1_parse.h
> +++ b/libavcodec/av1_parse.h
> @@ -130,6 +130,9 @@ static inline int parse_obu_header(const uint8_t *buf, int buf_size,
>      if (get_bits_left(&gb) < 0)
>          return AVERROR_INVALIDDATA;
>  
> +    if (*obu_size > (uint64_t)buf_size - get_bits_count(&gb) / 8)
> +        return AVERROR_INVALIDDATA;
> +
>      *start_pos = get_bits_count(&gb) / 8;
>  
>      size = *obu_size + *start_pos;

Right below this line there's the check

    if (size > INT_MAX)
        return AVERROR(ERANGE);

So i think you could just change it to "size > (int64_t)buf_size" and
achieve the same effect without adding an extra check.
Michael Niedermayer Oct. 14, 2018, 3:18 p.m.
On Sun, Oct 14, 2018 at 11:03:29AM -0300, James Almer wrote:
> On 10/14/2018 10:43 AM, Michael Niedermayer wrote:
> > Fixes: out of array read
> > Fixes: SIGSEGV_get_obu_bit_length_av1_parse
> > 
> > Found-by: keval shah <skeval65@gmail.com>
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/av1_parse.h | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/libavcodec/av1_parse.h b/libavcodec/av1_parse.h
> > index 276af33ba9..312d8825e1 100644
> > --- a/libavcodec/av1_parse.h
> > +++ b/libavcodec/av1_parse.h
> > @@ -130,6 +130,9 @@ static inline int parse_obu_header(const uint8_t *buf, int buf_size,
> >      if (get_bits_left(&gb) < 0)
> >          return AVERROR_INVALIDDATA;
> >  
> > +    if (*obu_size > (uint64_t)buf_size - get_bits_count(&gb) / 8)
> > +        return AVERROR_INVALIDDATA;
> > +
> >      *start_pos = get_bits_count(&gb) / 8;
> >  
> >      size = *obu_size + *start_pos;
> 
> Right below this line there's the check
> 
>     if (size > INT_MAX)
>         return AVERROR(ERANGE);
> 
> So i think you could just change it to "size > (int64_t)buf_size" and
> achieve the same effect without adding an extra check.

ive written it a bit overly defensive, not assuming any range limitation
of leb128().
But you are correct, ill simplify and repost it

thx


[...]
James Almer Oct. 14, 2018, 3:33 p.m.
On 10/14/2018 12:18 PM, Michael Niedermayer wrote:
> On Sun, Oct 14, 2018 at 11:03:29AM -0300, James Almer wrote:
>> On 10/14/2018 10:43 AM, Michael Niedermayer wrote:
>>> Fixes: out of array read
>>> Fixes: SIGSEGV_get_obu_bit_length_av1_parse
>>>
>>> Found-by: keval shah <skeval65@gmail.com>
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavcodec/av1_parse.h | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/libavcodec/av1_parse.h b/libavcodec/av1_parse.h
>>> index 276af33ba9..312d8825e1 100644
>>> --- a/libavcodec/av1_parse.h
>>> +++ b/libavcodec/av1_parse.h
>>> @@ -130,6 +130,9 @@ static inline int parse_obu_header(const uint8_t *buf, int buf_size,
>>>      if (get_bits_left(&gb) < 0)
>>>          return AVERROR_INVALIDDATA;
>>>  
>>> +    if (*obu_size > (uint64_t)buf_size - get_bits_count(&gb) / 8)
>>> +        return AVERROR_INVALIDDATA;
>>> +
>>>      *start_pos = get_bits_count(&gb) / 8;
>>>  
>>>      size = *obu_size + *start_pos;
>>
>> Right below this line there's the check
>>
>>     if (size > INT_MAX)
>>         return AVERROR(ERANGE);
>>
>> So i think you could just change it to "size > (int64_t)buf_size" and
>> achieve the same effect without adding an extra check.
> 
> ive written it a bit overly defensive, not assuming any range limitation
> of leb128().
> But you are correct, ill simplify and repost it

Make it return AVERROR_INVALIDDATA instead of ERANGE as well while at
it. Thanks.

> 
> thx
> 
> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Patch hide | download patch | download mbox

diff --git a/libavcodec/av1_parse.h b/libavcodec/av1_parse.h
index 276af33ba9..312d8825e1 100644
--- a/libavcodec/av1_parse.h
+++ b/libavcodec/av1_parse.h
@@ -130,6 +130,9 @@  static inline int parse_obu_header(const uint8_t *buf, int buf_size,
     if (get_bits_left(&gb) < 0)
         return AVERROR_INVALIDDATA;
 
+    if (*obu_size > (uint64_t)buf_size - get_bits_count(&gb) / 8)
+        return AVERROR_INVALIDDATA;
+
     *start_pos = get_bits_count(&gb) / 8;
 
     size = *obu_size + *start_pos;