Patching ffmpeg into shape

Preface: unless otherwise noted, the bugs discussed here were found via fuzzing by Will Dormann of CERT -- and my involvement was to fix them. In other news, I recently moved to work on the Chromium project / Google Chrome, which I'm very excited about. It is in this new role that this piece of work was conducted, as part of HTML5 features.

I recently fixed a lot of security bugs in ffmpeg, across a subset of the supported containers and codecs. The bugs represented a range of different C vulnerability classes, which I thought might make an interesting blog post.

1. Out-of-bounds array index read in vorbis_dec.c:

   
       mapping_setup->submap_floor[j]=get_bits(gb, 8);
       mapping_setup->submap_residue[j]=get_bits(gb, 8);
   ...
       floor=&vc->floors[mapping->submap_floor[0]];
   ...
       no_residue[i]=floor->decode(vc, &floor->data, ch_floor_ptr);
   

   
   The index usage is far from where it is read, making this trickier to find by code auditing. Note how this is exploitable, despite being an out-of-bounds read. The out-of-bounds memory is used as a function pointer! It's easy to forget that out-of-bounds reads can be very serious.
   


2. Off-by-one indexing error in vp3.c:

   
               for (j = 0; j < run_length; i++) {
                   if (i > s->coded_fragment_list_index)
                       return -1;
   
                   if (s->all_fragments[s->coded_fragment_list[i]].qpi == qpi) {
   

   
   The > should be >= otherwise there is an out-of-bounds read. In this instance, the out-of-bounds read is for an index that is used to read and write to another array, so you have possible memory corruption as a consequence.
   Found by me with some additional fuzzing.
   


3. Interesting pointer arithmetic bug in oggparsevorbis.c:

   
       while (p < end && n > 0) {
           const char *t, *v;
           int tl, vl;
   
           s = bytestream_get_le32(&p);
   
           if (end - p < s)
               break;
   
           t = p;
           p += s;
   

   
   The subtlety here is that bytestream_get_le32() advances the pointer passed to it, by sizeof(int). If the current position pointer p points to, say, 3 bytes of remaining buffer, then we have two problems. Firstly, the integer read will be compromised of one byte of out-of-bounds data. More seriously, the condition p > end will end up being true. Subsequently, a very large value of s may escape the check against end - p. This will lead to attacker-controlled out-of-bounds reads later.
   


4. Assignment vs. comparison operator mix-up in vorbis_dec.c:

   
       for(i=0;i<mapping->submaps;++i) {
           vorbis_residue *residue;
           uint_fast8_t ch=0;
   
           for(j=0;j<vc->audio_channels;++j) {
               if ((mapping->submaps==1) || (i=mapping->mux[j])) {
   

   
   Do you see it? This is actually a serious bug because these loops are writing to an output heap buffer, and the loops have been carefully checked so that they will terminate before they overflow said buffer! Unfortunately, assigning to the loop iterator from an attacker-controlled variable will permit the attacker to conduct a heap overflow.
   Found by me via code auditing.
   


5. Integer underflow leading to stack pointer wrap-around in vorbis_dec.c:

   
       uint_fast16_t n_to_read=vr->end-vr->begin;
       uint_fast16_t ptns_to_read=n_to_read/vr->partition_size;
       uint_fast8_t classifs[ptns_to_read*vc->audio_channels];
   

   
   All sorts of interesting stuff going on here. For a start, missing validations parsing the header earlier do not prevent begin > end, leading to an integer underflow. Furthermore, uint_fast16_t on my platform at least is a 32-bit wide type. This enables the calculation for the size of the stack-based array to result in any 32-bit value too. On a 32-bit platform, this can result in the stack pointer wrapping around and moving "up" rather than "down". A usually exploitable condition. Even creating a huge (but non-wrap-causing) stack frame can have consequences depending on whether your system does a simple calculation on the stack pointer, or is more careful to fault each new page in case an end-of-stack guard page exists.
   


6. Integer underflow by 1 in mov.c:

   
   static int mov_read_elst(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
   {
       MOVStreamContext *sc = c->fc->streams[c->fc->nb_streams-1]->priv_data;
   ...
               sc->time_offset = time != -1 ? time : -duration;
   

   
   The problem here is that it is assumed that nb_streams > 0 but no such condition is guaranteed if the attacker supplies an "elst" tag before supplying any tag that creates a stream. The result is that a pointer is used from an out-of-bounds location. Use of any invalid or corrupt pointer is usually an exploitable condition, of course.
   


7. Type confusion bug in mov.c / utils.c:
   (No code sample)
   The most interesting bug in my opinion. It triggers when a corrupt ordering of tags in the MOV container sets up some variables such "codec type" is e.g. VIDEO whereas "codec id" is e.g. MP3. Subsequently, some core code passes a pointer to a stack-allocated video structure to the the mp3 decoder. The mp3 decoder, assuming it has a pointer to the (larger) audio structure, happily causes a stack-based buffer overflow. Cool.
   Found by me with some additional fuzzing.
   


8. Other:
   This blog post got too long. Other bug classes included divide by zero, infinite looping, stack-based buffer overflow due to missing bounds check, classic integer overflow, and likely others. See the patches if you are interested: http://src.chromium.org/viewvc/chrome/trunk/deps/third_party/ffmpeg/patches/
   

Epilogue: The potential for bugs like these is why Chromium runs codecs inside its built-in sandbox. Although bugs inside the sandbox are still taken seriously, there are no longer of "Critical" severity. The real user damage on the web at this time is conducted by malware exploiting "Critical" severity bugs in some browser or plug-in.