[Ffmpeg-devel] h264, protection against corrupted data

Michael Niedermayer michaelni
Thu Jan 18 01:40:51 CET 2007


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


> 
> Some other are just verifying NULL pointer or index of an array. For example
> checking the return value of remove_short() which returns negative on
> failure and the return value was used as an array index right after.
> 
> I have also included 3 comments where it crashed one of which I don't know
> how it can easily be fixed (line 4195 on pic being invalid pointer). Please
> remove the crash comments if you don't like them.(I have attached a patch
> without them)

ive no problem with the comments but they should be in a seperate patch as
its a seperate thing (fix crashes vs. comments about crashes which havnt been
fixed) 
of course comments like

if(x==NULL) //added protection

are not ok as they are useless


> 
> Anyway I hope this tiny patch is welcome, Please don't think I'm criticizing
> h264.c, It is great and allow me and lots of other people to undergo
> interesting projects. Thanks.

patches are always welcome, note disscussuions about them should stay on
ffmpeg-dev


[...]

> @@ -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


>  
>  #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


>  
>  //printf("decoding esc\n");
>      si=di=0;
> @@ -3949,11 +3958,17 @@
>                              ref->pic_id= ref->frame_num;
>                      }else{
>                          pic_id= get_ue_golomb(&s->gb); //long_term_pic_idx
> -                        ref = h->long_ref[pic_id];
> -                        ref->pic_id= pic_id;
> -                        assert(ref->reference == 3);
> -                        assert(ref->long_ref);
> -                        i=0;
> +                        ref = h->long_ref[pic_id];    
> +                        if ( ref ){//added protection
> +                            ref->pic_id= pic_id;
> +                            assert(ref->reference == 3);
> +                            assert(ref->long_ref);
> +                            i=0;
> +                        }
> +                        else
> +                        {
> +                            i=-1;
> +                        }

the {} placement doesnt match the existing code


[...]
> @@ -4259,8 +4275,10 @@
>              if(pic) unreference_pic(h, pic);
>  
>              h->long_ref[ mmco[i].long_index ]= remove_short(h, mmco[i].short_frame_num);
> -            h->long_ref[ mmco[i].long_index ]->long_ref=1;
> -            h->long_ref_count++;
> +            if ( h->long_ref[ mmco[i].long_index ] ){ // added protection
> +               h->long_ref[ mmco[i].long_index ]->long_ref=1;
> +               h->long_ref_count++;
> +            }

this is indented by 3 spaces while 4 is used everywhere else in ffmpeg
also the comment is not usefull outside this patch -> it should not be here


[...]
> @@ -7683,6 +7701,13 @@
>      get_bits(&s->gb, 4); // reserved
>      level_idc= get_bits(&s->gb, 8);
>      sps_id= get_ue_golomb(&s->gb);
> +    
> +    // if we get the wrong sps_id, we get a wrong sps pointer ! ouch.
> +    if ( sps_id < 0 || sps_id >= MAX_SPS_COUNT ){
> +        // ok it has gone out of hand, someone is sending us bad stuff.
> +        av_log(h->s.avctx, AV_LOG_ERROR, "illegal sps_id (%d)\n", sps_id);
> +        return -1;
> +    }

a sps_id >= MAX_SPS_COUNT check with unsigned int sps_id is ok the sps_id<0
is not


>  
>      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?


> +        
>          ptr= decode_nal(h, buf + buf_index, &dst_length, &consumed, h->is_avc ? nalsize : buf_size - buf_index);
>          while(ptr[dst_length - 1] == 0 && dst_length > 1)
>              dst_length--;
> @@ -8122,7 +8152,7 @@
>          execute_ref_pic_marking(h, h->mmco, h->mmco_index);
>  
>      ff_er_frame_end(s);
> -
> +    
>      MPV_frame_end(s);

cosmetic change


[...]
> @@ -8530,7 +8561,8 @@
>      decode_end,
>      decode_frame,
>      /*CODEC_CAP_DRAW_HORIZ_BAND |*/ CODEC_CAP_DR1 | CODEC_CAP_TRUNCATED | CODEC_CAP_DELAY,
> -    .flush= flush_dpb,
> +    NULL, //next
> +    /*.flush=*/ flush_dpb

unrelated and not ok

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No evil is honorable: but death is honorable; therefore death is not evil.
-- Citium Zeno
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070118/4deb7de9/attachment.pgp>



More information about the ffmpeg-devel mailing list