| Message ID | 20181014134356.31500-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Headers | show |
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.
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 [...]
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 >
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;
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(+)