[FFmpeg-devel] Security issues?

[Michael Niedermayer]

On Wed, Sep 23, 2009 at 01:46:13PM +0200, Reimar D?ffinger wrote:
> On Wed, Sep 23, 2009 at 01:27:21PM +0200, Michael Niedermayer wrote:
> > 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
> 
> Something like this maybe?

yes, also note we have somethig similar in nutdec.c (GET_V) so maybe the
macro should go to libavutil


> Index: libavcodec/vorbis_dec.c
> ===================================================================
> --- libavcodec/vorbis_dec.c     (revision 19987)
> +++ libavcodec/vorbis_dec.c     (working copy)
> @@ -162,6 +162,13 @@
>  #define BARK(x) \
>      (13.1f*atan(0.00074f*(x))+2.24f*atan(1.85e-8f*(x)*(x))+1e-4f*(x))
>  
> +
> +#define VALIDATE_INDEX(ctx, idx, limit, onerror) \

> +    if (idx >= limit) {\

the check should be an argument to the macro


> +        av_log(ctx, AV_LOG_ERROR, "Index value %d out of range (0 - %d) for "#idx "\n", idx, limit);\
> +       onerror\
> +    }
> +
>  static float vorbisfloat2float(uint_fast32_t val) {
>      double mant=val&0x1fffff;
>      long exp=(val&0x7fe00000L)>>21;
> @@ -696,10 +703,7 @@
>              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));
> -                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;
> -                }
> +                VALIDATE_INDEX(vc->avccontext, mapping_setup->magnitude[j], vc->audio_channels, 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;
> 
> > > 
> > > ===================================================
> > > 
> > > 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
> 
> Well, there still is the size of the whole dref atom.
> Maybe continuing to parse the dref indeed makes no sense, but I think -1
> will stop processing for the whole file, which should not be necessary
> and can be avoided by returning 0 (I haven't checked the code too
> closely though).

ahh, ok, makes sense

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

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- 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/c4c489b9/attachment.pgp>