515889 - Possible overflow in Vorbis_cookbook.c

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Reed Loden [:reed]]

Dan Kaminsky reported an issue with Vorbis to Monty of xiph.org that could possibly affect Firefox.

--------------------
 Monty,

    Looking through your code as embedded in FF3.5, I see in Vorbis_cookbook.c something that might be a problem:

 /* unpacks a codebook from the packet buffer into the codebook struct,
    readies the codebook auxiliary structures for decode *************/
 int vorbis_staticbook_unpack(oggpack_buffer *opb,static_codebook *s){
 ...
  /* first the basic parameters */
   s->dim=oggpack_read(opb,16); // unbound, 0 to 2**16-1
   s->entries=oggpack_read(opb,24); // unbound, 0 to 2**24-1 (must not be 1)
   if(s->entries==-1)goto _eofout;
 ...
   switch((int)oggpack_read(opb,1)){
   case 0:
 ...
   case 1:
 ...
   /* Do we have a mapping to unpack? */
   switch((s->maptype=oggpack_read(opb,4))){
   case 0:
 ...
   case 1: case 2:
       switch(s->maptype){
       case 1:
 ...
       case 2:
     quantvals=s->entries*s->dim; // 2**16-1 * 2**24-1 overflows, and probably affects things elsewhere in code, but...
     break;
       }

       /* quantized values */
       s->quantlist=_ogg_malloc(sizeof(*s->quantlist)*quantvals); // this is the useful overflow
       for(i=0;i<quantvals;i++)
     s->quantlist[i]=oggpack_read(opb,s->q_quant); //will read arbitrary values into RAM, 41 41 00 00 style.

 Mind taking a look?  Attacker seems to have two options; first, write lots of memory, second, make quantvals negative thus leaving the quantization tables unallocated (and skipping any likely AV's).

Comment 1

[Daniel Veditz [:dveditz]]

Firefox 3.0 didn't have <video>, not needed for 1.9.0.x

Comment 2

[Samuel Sidler (old account; do not CC)]

Need comments here on if this is actually exploitable before we know whether we need to block on it for 1.9.1.

Comment 3

[Dan Kaminsky]

No, this one's pretty clean, the overflow, alloc, and copy from attacker controlled content all happen in one function with no other checks that could possibly interfere.

This is *also* fixed upstream in Xiph code, with:

  if(_ilog(s->dim)+_ilog(s->entries)>24)goto _eofout;

...inside of http://svn.xiph.org/trunk/vorbis/lib/codebook.c .

Should definitely block on this one.  Adding Tim for his comments.

Comment 4

[Daniel Veditz [:dveditz]]

Bug 501279 incorporated the newer vorbis into 1.9.2
http://mxr.mozilla.org/mozilla1.9.2/source/media/libvorbis/lib/vorbis_codebook.c#149

We're working on getting that into 1.9.1.x (Firefox 3.5.x)

Comment 5

[Al Billings [:abillings - ex-MoCo]]

Is this marked fixed because the newer vorbis has been checked in?

Comment 6

[Al Billings [:abillings - ex-MoCo]]

Anyone want to answer my question?

Comment 7

[Matthew Gregan [:kinetik]]

Yes.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Marking verified for 1.9.1 then.

Comment 9

[Tomas Hoger]

(In reply to comment #3)
> This is *also* fixed upstream in Xiph code, with:
> 
>   if(_ilog(s->dim)+_ilog(s->entries)>24)goto _eofout;
> 
> ...inside of http://svn.xiph.org/trunk/vorbis/lib/codebook.c .

... which was added in libvorbis SVN r14604 and is also known as CVE-2008-1423 (previously reported by Will Drewry), unless I'm mistaken.