[Ffmpeg-devel] h264, protection against corrupted data (second try patch)

[Frank]

Hello

On 1/17/07, Michael Niedermayer <michaelni at gmx.at> wrote:
>
> Hi
>
> On Wed, Jan 17, 2007 at 02:22:07PM -0500, Frank wrote:
> > Attached is a patch for h264.c which are modifications I use to prevent
> > crashes. I mean all modified lines are where I had a crash and debugged
> to
> > find out where it happened, There are probably other places where index
> are
> > applied to array which could result in overflow (or negative array
> index). I
> > read the post about fuzzer bugs (zzuf application) posted January 15th
> 2007
> > and it looks like decoders are really sensible to corrupted data and it
> > convinced me to re-post my patch and also mention it would be a good
> idea to
> > increase array index verifications in h264.c (from my point of view of
> > course)
> >
> > There is one crash which is due to sps_id being negative. I submited
> this
> > fix several weeks ago and it was rejected because apparently sps_id
> cannot
> > be negative. To reply to that I would say from a programming point of
> view
> > it is an "int" and when the 32bit value from the byte stream is bigger
> than
> > INT_MAX, it goes negative. Unless get_bits() shave bits to INT_MAX ?
>
> sps_id should then be changed to unsigned int


done.
I've also changed pps_id to unsigned int while at it.
There was one issue where a pps_id was assigned to an int so I did:
int dequant_coeff_pps = (int) pps_id;
which I believe is safe since pps_id is validated as being lower than 256
when assigned and if uninitialized will be zero.
The cast is compile-time difference and doesn't add run-time convertion.
Anyway I don't see a lot of these cast in ffmpeg so this is why I mention it
(in case it is not ok).


> > @@ -1400,6 +1400,11 @@
> >      const int is_b8x8 = IS_8X8(*mb_type);
> >      int sub_mb_type;
> >      int i8, i4;
> > +
> > +    if ( h->ref_list[1][0].mb_type == 0 ){
> > +        //fixes a crash
> > +        return;
> > +    }
>
> as this is dereferenced a few lines above, this change of course is not
> correct
> also a simple return is no solution for a missing reference frame,
> possible
> solutions would be to either drop b frames which refer to missing frames,
> or
> to somehow change the reference and motion vectors to some guess simply
> returning randomly is no solution


I have removed this from the patch since I'm not sure how to fix it right
now.
I will work on it if I can manage to get a recorded stream that triggers the
crash I'm trying to circumvent.

>
> >  #define MB_TYPE_16x16_OR_INTRA
> (MB_TYPE_16x16|MB_TYPE_INTRA4x4|MB_TYPE_INTRA16x16|MB_TYPE_INTRA_PCM)
> >      if(IS_8X8(mb_type_col) && !h->sps.direct_8x8_inference_flag){
> > @@ -1779,6 +1784,10 @@
> >
> >      h->rbsp_buffer= av_fast_realloc(h->rbsp_buffer,
> &h->rbsp_buffer_size, length);
> >      dst= h->rbsp_buffer;
> > +
> > +    if ( dst == NULL ){ // i do length=0 but anyone can modify this to
> an appropriate return
> > +        length=0;
> > +    }
>
> well you have to change this to a appropriate return before it can be
> applied
> we never accept code which is half wrong


I suggested an appropriate return but it fact there was none since the
pointer was used right after the function call. So I added a verification
that the returned pointer is non-NULL and return NULL when realloc failed.

>
> >      sps= &h->sps_buffer[ sps_id ];
> >      sps->profile_idc= profile_idc;
> > @@ -8009,6 +8034,11 @@
> >          buf_index+=3;
> >        }
> >
> > +        if ( h->is_avc && nalsize > 7100000 ){       // FIXME add a
> max_nal_size
> > +            // added when problem seeking in a h264 mov file.
> > +            return -1;
> > +        }
>
> why?


When h->is_avc is true, the nalsize is read from byte stream. Usually when
it is a network read, the packet size is trusted good. Or when it is parsed,
usually the parser does a good job at getting a complete nal and its size.
During a seek, we trust the mov table of content to be accurate and the seek
to succeed. Since nalsize is read from byte stream, and let say the seek
went randomly somewhere or the value read was corrupted, to trust nalsize is
a hazardous and a read access exception will occur if nalsize is bigger than
buffer. Right now nalsize is verified to be above zero after it is read. I
had a crash where it was very large so I guessed it should also be checked
to be below some max nal size. Also I modified the patch so verification is
done at same place as being above zero. I also modified verification so it
is compared to buf_size instead of an theorical max nal size figure.

- if(nalsize <= 1)
+ if(nalsize <= 1 || nalsize > buf_size)

Thanks,
Francois
-------------- next part --------------
A non-text attachment was scrubbed...
Name: h264.c.18jan2007.diff
Type: application/octet-stream
Size: 6815 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070118/116552df/attachment.obj>