[vlc] [vendor-sec] integer overflow leading to heap overflow in vlc 0.8.6e (mp4 demuxer)

[Drew Yao]

I reported this earlier this month.  If you check recent git revisions  
of that file, it's been fixed there for a while.  Sorry I forgot to  
mention it to vendor-sec.

http://trac.videolan.org/vlc/changeset/09572892df7e72c0d4e598c0b5e076cf330d8b0a

---
Drew Yao
Apple Product Security




On Mar 23, 2008, at 5:13 AM, Nico Golde wrote:
> Hi,
> I think I found a security issue in the latest vlc release (0.8.6e).
>
> Quoting modules/demuxer/mp4/libmp4.c, MP4_ReadBox_rdrf() function:
>
>  1954  static int MP4_ReadBox_rdrf( stream_t *p_stream, MP4_Box_t  
> *p_box )
>  1955  {
>  1956      uint32_t i_len;
>  1957      MP4_READBOX_ENTER( MP4_Box_data_rdrf_t );
>  1958
>  1959      MP4_GETVERSIONFLAGS( p_box->data.p_rdrf );
>  1960      MP4_GETFOURCC( p_box->data.p_rdrf->i_ref_type );
>  1961      MP4_GET4BYTES( i_len );
>  1962      if( i_len > 0 )
>  1963      {
>  1964          uint32_t i;
>  1965          p_box->data.p_rdrf->psz_ref = malloc( i_len  + 1);
>  1966          for( i = 0; i < i_len; i++ )
>  1967          {
>  1968              MP4_GET1BYTE( p_box->data.p_rdrf->psz_ref[i] );
>  1969          }
>  1970          p_box->data.p_rdrf->psz_ref[i_len] = '\0';
>  1971      }
>  1972      else
>
> In line 1961 MP4_GET4BYTES reads the atom length of the mov file as  
> specified
> in the apple quicktime standard and stores the value in the i_len  
> variable.
> On positive values it then allocates memory to store that atom data  
> in a buffer.
> There is a problem with this code here:
> 1965          p_box->data.p_rdrf->psz_ref = malloc( i_len  + 1);
>
> When supplied 0xFFFFFFFF as the atom lenght i_len + 1 will overflow  
> and resulting
> in malloc allocating the smallest possible chunk because malloc is  
> called with a length
> argument of 0. It will not fail but it won't allocate the needed  
> memory.
> The for-loop in 1966 will then happily copy a lot more data into the  
> buffer.
> When exploited this could possibly lead to code execution.
>
> Btw since i_len is of type uint32_t the else branch will be never  
> used.
>
> A check for malloc returning NULL doesn't hurt either.
>
> A proof of concept mov file can be found on:
> http://nion.modprobe.de/la.mov
>
> Kind regards
> Nico
>
> -- 
> Nico Golde - http://www.ngolde.de - nion at jabber.ccc.de - GPG:  
> 0x73647CFF
> For security reasons, all text in this mail is double-rot13 encrypted.