[FFmpeg-devel] Security issues?

[Michael Niedermayer]

On Wed, Sep 23, 2009 at 12:31:51PM +0200, Reimar D?ffinger wrote:
[...]
> ================================================
> 
> 26_vorbis_mag_angle_index:
> --- libavcodec/vorbis_dec.c.orig14	2009-08-28 11:07:19.000000000 -0700
> +++ libavcodec/vorbis_dec.c	2009-08-28 11:28:40.000000000 -0700
> @@ -729,7 +729,14 @@
>              for(j=0;j<mapping_setup->coupling_steps;++j) {
>                  mapping_setup->magnitude[j]=get_bits(gb, ilog(vc->audio_channels-1));
>                  mapping_setup->angle[j]=get_bits(gb, ilog(vc->audio_channels-1));
> -                // FIXME: sanity checks
> +                if (mapping_setup->magnitude[j]>=vc->audio_channels) {
> +                    av_log(vc->avccontext, AV_LOG_ERROR, "magnitude channel %d out of range. \n", mapping_setup->magnitude[j]);
> +                    return 1;
> +                }
> +                if (mapping_setup->angle[j]>=vc->audio_channels) {
> +                    av_log(vc->avccontext, AV_LOG_ERROR, "angle channel %d out of range. \n", mapping_setup->angle[j]);
> +                    return 1;
> +                }
>              }
>          } else {
>              mapping_setup->coupling_steps=0;
> 
> IMO only use one if/message for both.

i already applied this one
btw, i think instead of ambigous messages, we could add a macro that keeps
src bloat down


[...]

> 
> ===================================================
> 
> 29_mov_dref_looping:
> --- libavformat/mov.c.orig2	2009-08-31 15:41:36.000000000 -0700
> +++ libavformat/mov.c	2009-08-31 15:55:36.000000000 -0700
> @@ -261,6 +261,8 @@
>          MOVDref *dref = &sc->drefs[i];
>          uint32_t size = get_be32(pb);
>          int64_t next = url_ftell(pb) + size - 4;
> +        if (size < 8)
> +            return -1;
>  
>          dref->type = get_le32(pb);
>          get_be32(pb); // version + flags
> 
> I do not like failing hard, using FFMAX to make sure size is at least
> e.g. 4 would be better IMO (no idea what the minimum size actually is).

i disagree, size is what tells us where the next element is, if its wrong
continuing is very unlikely to work


> 
> =======================================================
> 
> 31_mp3_outlen:
> --- libavcodec/mpegaudiodec.c.orig	2009-08-31 23:19:15.000000000 -0700
> +++ libavcodec/mpegaudiodec.c	2009-08-31 23:20:05.000000000 -0700
> @@ -2294,8 +2294,10 @@
>          *data_size = out_size;
>          avctx->sample_rate = s->sample_rate;
>          //FIXME maybe move the other codec info stuff from above here too
> -    }else
> +    }else{
>          av_log(avctx, AV_LOG_DEBUG, "Error while decoding MPEG audio frame.\n"); //FIXME return -1 / but also return the number of bytes consumed
> +        *data_size = 0;
> +    }
>      s->frame_size = 0;
>      return buf_size + skipped;
>  }
> 
> *data_size = 0 IMO should be done right at the top of the decode_frame

nope, data_size should first be checked, iam working on that ..


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090923/bc33a7d4/attachment.pgp>