[FFmpeg-devel] [PATCH] Add some buffer sanity checks to indeo3

[Alex Converse]

On Tue, Feb 17, 2009 at 8:15 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Tue, Feb 17, 2009 at 07:06:26PM -0500, Alex Converse wrote:
>> On Tue, Feb 17, 2009 at 6:22 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> >
>> > On Tue, Feb 17, 2009 at 04:58:31PM -0500, Alex Converse wrote:
>> > > On Tue, Feb 17, 2009 at 4:05 PM, Michael Niedermayer <michaelni at gmx.at>wrote:
>> > >
>> > > > On Mon, Feb 16, 2009 at 02:13:02PM -0500, Alex Converse wrote:
>> > > > > This helps with but doesn't completely fix 1-dog.avi
>> > > > >
>> > > > > Regards,
>> > > > > Alex Converse
>> > > >
>> > > > > diff --git a/libavcodec/indeo3.c b/libavcodec/indeo3.c
>> > > > > index 6173c6f..3663d40 100644
>> > > > > --- a/libavcodec/indeo3.c
>> > > > > +++ b/libavcodec/indeo3.c
>> > > > > @@ -975,7 +975,7 @@ static av_cold int indeo3_decode_init(AVCodecContext
>> > > > *avctx)
>> > > > >      return ret;
>> > > > >  }
>> > > > >
>> > > > > -static unsigned long iv_decode_frame(Indeo3DecodeContext *s,
>> > > > > +static int iv_decode_frame(Indeo3DecodeContext *s,
>> > > > >                                       const uint8_t *buf, int buf_size)
>> > > > >  {
>> > > > >      unsigned int image_width, image_height,
>> > > >
>> > > > > @@ -1006,6 +1006,11 @@ static unsigned long
>> > > > iv_decode_frame(Indeo3DecodeContext *s,
>> > > > >      hdr_pos = buf_pos;
>> > > > >      if(data_size == 0x80) return 4;
>> > > > >
>> > > > > +    if(y_offset >= buf_size - 16 || v_offset >= buf_size - 16 ||
>> > > > u_offset >= buf_size -16) {
>> > > > > +        av_log(s->avctx, AV_LOG_ERROR, "y/u/v offset outside buffer\n");
>> > > > > +        return -1;
>> > > > > +    }
>> > > > > +
>> > > > >      if(flags & 0x200) {
>> > > > >          s->cur_frame = s->iv_frame + 1;
>> > > > >          s->ref_frame = s->iv_frame;
>> > > >
>> > > > FFMAX3(y_offset, u_offset, v_offset) >= buf_size - 16
>> > > >
>> > > >
>> > > >
>> > > > > @@ -1016,6 +1021,10 @@ static unsigned long
>> > > > iv_decode_frame(Indeo3DecodeContext *s,
>> > > > >
>> > > > >      buf_pos = buf + 16 + y_offset;
>> > > > >      mc_vector_count = bytestream_get_le32(&buf_pos);
>> > > > > +    if(16 + y_offset + 2*mc_vector_count >= buf_size) {
>> > > > > +        av_log(s->avctx, AV_LOG_ERROR, "mc_vector_count too large\n");
>> > > > > +        return -1;
>> > > > > +    }
>> > > > >
>> > > > >      iv_Decode_Chunk(s, s->cur_frame->Ybuf, s->ref_frame->Ybuf,
>> > > > image_width,
>> > > > >                      image_height, buf_pos + mc_vector_count * 2,
>> > > > cb_offset, hdr_pos, buf_pos,
>> > > >
>> > > > can overflow
>> > > >
>> > > >
>> > > > > @@ -1026,6 +1035,10 @@ static unsigned long
>> > > > iv_decode_frame(Indeo3DecodeContext *s,
>> > > > >
>> > > > >          buf_pos = buf + 16 + v_offset;
>> > > > >          mc_vector_count = bytestream_get_le32(&buf_pos);
>> > > > > +        if(16 + v_offset + 2*mc_vector_count >= buf_size) {
>> > > > > +            av_log(s->avctx, AV_LOG_ERROR, "mc_vector_count too
>> > > > large\n");
>> > > > > +            return -1;
>> > > > > +        }
>> > > > >
>> > > > >          iv_Decode_Chunk(s, s->cur_frame->Vbuf, s->ref_frame->Vbuf,
>> > > > chroma_width,
>> > > > >                  chroma_height, buf_pos + mc_vector_count * 2, cb_offset,
>> > > > hdr_pos, buf_pos,
>> > > > > @@ -1033,6 +1046,10 @@ static unsigned long
>> > > > iv_decode_frame(Indeo3DecodeContext *s,
>> > > > >
>> > > > >          buf_pos = buf + 16 + u_offset;
>> > > > >          mc_vector_count = bytestream_get_le32(&buf_pos);
>> > > > > +        if(16 + u_offset + 2*mc_vector_count >= buf_size) {
>> > > > > +            av_log(s->avctx, AV_LOG_ERROR, "mc_vector_count too
>> > > > large\n");
>> > > > > +            return -1;
>> > > > > +        }
>> > > > >
>> > > > >          iv_Decode_Chunk(s, s->cur_frame->Ubuf, s->ref_frame->Ubuf,
>> > > > chroma_width,
>> > > > >                  chroma_height, buf_pos + mc_vector_count * 2, cb_offset,
>> > > > hdr_pos, buf_pos,
>> > > >
>> > > > so can these
>> > >
>> > >
>> > > I'm aware iv_Decode_Chunk() can still overflow. It's a big monolithic
>> > > function with no documentation. I didn't have the time nor the energy to try
>> > > to unwind and make sense of it. Still these checks lessen the exposure of
>> > > the unsafe code to bad input.
>> >
>> > your very checks overflow
>> > 2*mc_vector_count being a example
>> >
>> > i am not asking you o fix other overflows in the code but the checks you add
>> > should not contain overflows themselfs
>> >
>>
>> Ahh, arithmetic overflow not buffer overflow.
>>
>> It feels a little awkward but I think I fixed it.
>>
>> Also whose crazy idea at Intel was it to make all those fields 32-bit?
>> That man deserves a medal.
> [...]
>> @@ -1016,6 +1021,10 @@ static unsigned long iv_decode_frame(Indeo3DecodeContext *s,
>>
>>      buf_pos = buf + 16 + y_offset;
>>      mc_vector_count = bytestream_get_le32(&buf_pos);
>> +    if(mc_vector_count > LONG_MAX || 2*mc_vector_count >= buf_size-16-y_offset) {
>> +        av_log(s->avctx, AV_LOG_ERROR, "mc_vector_count too large\n");
>> +        return -1;
>> +    }
>
> 2LL*mc_vector_count >= buf_size-16-y_offset
> should work too and be prettier
>
> [...]

Fixed
-------------- next part --------------
A non-text attachment was scrubbed...
Name: indeo3-buffer-sanity.diff
Type: text/x-diff
Size: 2755 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090219/6c09120d/attachment.diff>