[FFmpeg-devel] Security issues?

[Reimar Döffinger]

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?
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) {\
+        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).