A previous post analyzed a stack buffer-overflow in the parse_master function of VLC <=v0.9.4. parse_master is susceptible to another vulnerability, this time of the heap-overflow variety.

* Following this analysis requires some understanding of Intel assembly and basic reverse engineering concepts.

Throughout the analysis below, portions of the ty.c source code are referenced using a bracketed “[1]” annotation. This source code and all annotations are provided at the bottom of the page. In most cases, code snippets are also provided directly below the paragraph where the code is referenced.


vlc

Explanation of Source Code

When VLC plays a .ty or any other file and encounters a certain sequence of bytes [13] it calls the parse_master function with p_demux as an argument [15]. p_demux is the remaining (unprocessed) bytes of the input video file. The peek function [14] compares the current 4 bytes in the input stream to the magic bytes [13] and then rewinds p_demux to the offset beginning with those same bytes (in other words, if the magic bytes are encountered, parse_master is passed the input stream (p_demux) starting with the offset of the magic bytes).

#define TIVO_PES_FILEID   ( 0xf5467abd ) [13]
...
/* check if it's a PART Header */
    if( U32_AT( &p_peek[ 0 ] ) == TIVO_PES_FILEID )	[14]
    {
        /* parse master chunk */
        parse_master(p_demux); [15]

As you can see in the image below, the byte stream begins with the magic bytes “F5 46 7a bd”.

Video.ty+ Bytes

The parse_master function begins with declaring a series of variables including an array of 32 8-bit integers (mst_buf) [1] as well as two 32-bit integers (i and i_map_size) [2]. Further down, there is a call to thhe stream_Read function which reads 32 bytes from the input stream into mst_buf [3]. The following line [4], sets i_map_size to the 32-bit value located at mst_buf[20]. The variable i is then initialized [6] to the 32-bit value at the end of the mst_buf buffer (mst_buf[28]). Finally, the i_seq_table_size data element in the p_sys structure is set to the result of the expression i / (8 + i_map_size) [7].

demux_sys_t *p_sys = p_demux->p_sys;
uint8_t mst_buf[32]; [1]
int i, i_map_size; [2]
...
stream_Read(p_demux->s, mst_buf, 32); [3]
i_map_size = U32_AT(&mst_buf[20]);  /* size of bitmask, in bytes */[4]
p_sys->i_bits_per_seq_entry = i_map_size * 8;
i = U32_AT(&mst_buf[28]);   /* size of SEQ table, in bytes */	[6]
p_sys->i_seq_table_size = i / (8 + i_map_size);	[7]

After these initial variables are initialized, a malloc call is made [8] with the size argument passed as i_seq_table_size * sizeof(ty_seq_table_t) (the size of the ty_seq_table_t data element is 16). The resulting memory pointer is stored in seq_table.

p_sys->seq_table = malloc(p_sys->i_seq_table_size * sizeof(ty_seq_table_t)); [8]

Further down in the function, a memcpy call is made which takes i_map_size bytes from the mst_buf buffer and writes to the memory location pointed to by seq_table (the pointer returned from our previous malloc call [12]).

memcpy(p_sys->seq_table[i].chunk_bitmask, &mst_buf[8], i_map_size);	[12]

Source Code Vulnerability Analysis

With an understanding of the source code, let’s analyze the vulnerability… Given i_map_size is a signed integer [1], if we set it to a negative number, say -1 (or FFFFFFFFh) [4] and set i to a value of 7 [6], we can get an i_seq_table_size equal to 1 [7]. Keep in mind, we can set the values of i_map_size and i arbitrarily [4] [6] since these values are parsed directly out of mst_buf which comes from user-supplied input. Now, when malloc is called [8] the size will be 16 (i_seq_table_size which is 1 multiplied by _sizeof(ty_seq_table_t) which is 16) which will return a pointer to a memory region with at-least 16 bytes of memory. Of note here is this is a relatively SMALL memory region (one that would be easier to overflow).

uint8_t mst_buf[32]; [1]
...
i_map_size = U32_AT(&mst_buf[20]);  /* size of bitmask, in bytes */	[4]
p_sys->i_bits_per_seq_entry = i_map_size * 8;
i = U32_AT(&mst_buf[28]);   /* size of SEQ table, in bytes */	[6]
p_sys->i_seq_table_size = i / (8 + i_map_size);	[7]

/* parse all the entries */
p_sys->seq_table = malloc(p_sys->i_seq_table_size * sizeof(ty_seq_table_t)); [8]

Within the for loop declaration [9], we see it should execute i_seq_table_size amount of times which we know is 1 (so it should only iterate once through). Unlike the stack-overflow condition seen in a previous post, the stream_Read call within the for loop should execute with no issues (no overflow condition) as it is merely writing 7 bytes from the input stream into mst_buf [10]. In order to bypass the if condition [11] (which must be done to get to the memcpy function), i_map_size must be <= 8, which we know it is as we had previously set it to -1 (FFFFFFFFh). Finally, we get to the memcpy call [12] which writes FFFFFFFFh bytes from mst_buf into the memory location pointed to by seq_table. Since memcpy uses the FFFFFFFFh size value as an unsigned value, this is a very large amount of data it attempts to write into memory which overflows the allocated memory buffer which was only 16 when first passed to the malloc function earlier. This will result in an access violation and the program crashes due to overflowing the heap buffer!

for (i=0; i<p_sys->i_seq_table_size; i++) { [9]
    stream_Read(p_demux->s, mst_buf, 8 + i_map_size); [10]
    p_sys->seq_table[i].l_timestamp = U64_AT(&mst_buf[0]);
    if (i_map_size > 8) {	[11]
        msg_Err(p_demux, "Unsupported SEQ bitmap size in master chunk");
        memset(p_sys->seq_table[i].chunk_bitmask, i_map_size, 0);
    } else {
        memcpy(p_sys->seq_table[i].chunk_bitmask, &mst_buf[8], i_map_size);	[12]

Assembly Code Vulnerability Analysis

Analysis of the vulnerability continues by inspecting the disassembled code…

Initializing i_map_size variable from the mst_buf buffer

After the first of the two stream_Read calls (0x61401C1F), the FFFFFFFFh value passed in via the user inputted .ty file is loaded (via instructions 0x61401C24-0x61401C62) onto the stack and stored at offset 0x0629FB04 (ESP+A0).

61401C24   0FB6B424 D400000>MOVZX ESI,BYTE PTR SS:[ESP+D4]
61401C2C   0FB69C24 D500000>MOVZX EBX,BYTE PTR SS:[ESP+D5]
61401C34   0FB68C24 D700000>MOVZX ECX,BYTE PTR SS:[ESP+D7]
61401C3C   0FB69424 D600000>MOVZX EDX,BYTE PTR SS:[ESP+D6] ;these 4 grab FFFFFFFF from mst_buf stored on stack
61401C44   C1E6 18          SHL ESI,18
61401C47   89B424 A0000000  MOV DWORD PTR SS:[ESP+A0],ESI
61401C4E   C1E3 10          SHL EBX,10
61401C51   099C24 A0000000  OR DWORD PTR SS:[ESP+A0],EBX
61401C58   C1E2 08          SHL EDX,8
61401C5B   098C24 A0000000  OR DWORD PTR SS:[ESP+A0],ECX ;these 6 instructions convert endianness of bytes from input buffer
61401C62   099424 A0000000  OR DWORD PTR SS:[ESP+A0],EDX ;storing in 060BFB04

Initializing i variable from the mst_buf buffer

The i variable has the user inputted value loaded into it from the mst_buf array. This value is stored on the stack at ESP+A4

61401C83   0FB68424 DC00000>MOVZX EAX,BYTE PTR SS:[ESP+DC]
61401C8B   0FB6B424 DD00000>MOVZX ESI,BYTE PTR SS:[ESP+DD]
61401C93   0FB69C24 DF00000>MOVZX EBX,BYTE PTR SS:[ESP+DF]
61401C9B   0FB68C24 DE00000>MOVZX ECX,BYTE PTR SS:[ESP+DE] ;grabs DWORD from input buffer
61401CA3   C1E0 18          SHL EAX,18
61401CA6   C1E6 10          SHL ESI,10
61401CA9   09F0             OR EAX,ESI
61401CAB   C1E1 08          SHL ECX,8
61401CAE   09D8             OR EAX,EBX
61401CB0   09C8             OR EAX,ECX
61401CB2   89BC24 A4000000  MOV DWORD PTR SS:[ESP+A4],EDI ;store i value on stack

i_seq_table_size expression

At instruction 0x61401C70 the i_map_size value is loaded into EDI from the stack. 8 is added to it and then it is stored at an address on the stack.

61401C70   8BBC24 A0000000  MOV EDI,DWORD PTR SS:[ESP+A0]

61401C80   83C7 08          ADD EDI,8

61401CB2   89BC24 A4000000  MOV DWORD PTR SS:[ESP+A4],EDI

The stack now looks like…
0629FB04 FFFFFFFF ÿÿÿÿ
0629FB08 00000007 …

From here, the rest of i_seq_table_size is calculated.

61401CB9   99               CDQ ; sign extend EAX into EDX
61401CBA   F7BC24 A4000000  IDIV DWORD PTR SS:[ESP+A4]
61401CC1   8985 C8BE0000    MOV DWORD PTR SS:[EBP+BEC8],EAX

malloc call

EAX, now with a value of 1 is shifted left to get a value of 10h (which is 16) and malloc is called with this size value. (Remember, malloc was called with a value of 16 as this is the size of ty_seq_table_t.)

61401CBA   F7BC24 A4000000  IDIV DWORD PTR SS:[ESP+A4] ; SETS EAX to 1

61401CC7   C1E0 04          SHL EAX,4
61401CCA   890424           MOV DWORD PTR SS:[ESP],EAX
61401CCD   E8 4E580000      CALL <JMP.&msvcrt.malloc>

malloc returns with a memory pointer in EAX of (in my case it is 0x04909420.)

No stream_Read overwrite issue

The following assembly instructions set up the second stream_Read call which has a size value parameter of 7 which will not overwrite the mst_buf size of 32.

61401D4A   894C24 04        MOV DWORD PTR SS:[ESP+4],ECX ;pointer to mst_buf
61401D4E   897424 08        MOV DWORD PTR SS:[ESP+8],ESI ;size value of 7

61401D57   893C24           MOV DWORD PTR SS:[ESP],EDI ;pointer to P_demux stream
61401D5A   E8 31510000      CALL <JMP.&libvlccore.stream_Read>

Bypass if statement

The following shows the if statement in assembly ensuring that i_map_size is not greater than 8.

61401EA5   83BC24 A0000000 >CMP DWORD PTR SS:[ESP+A0],8
61401EAD   894C3B 04        MOV DWORD PTR DS:[EBX+EDI+4],ECX
61401EB1  ^0F8F 3AFEFFFF    JG libty_pl.61401CF1 ; This jump is not taken as it is not greater

memcpy call

The following instructions set up and execute the memcpy.

61401EBF   8B9424 A0000000  MOV EDX,DWORD PTR SS:[ESP+A0] ; FFFFFFFF into EDX
61401EC6   01F1             ADD ECX,ESI ; ECX is 0 so this sticks ESI (malloc pointer) into ECX
61401EC8   83FA 07          CMP EDX,7 ; Compares EDX FFFFFFFF to 7
61401ECB   8D79 08          LEA EDI,DWORD PTR DS:[ECX+8] ;
61401ECE   8DB424 C8000000  LEA ESI,DWORD PTR SS:[ESP+C8] ;LEA on ESI which has malloc result pointer
61401ED5   76 29            JBE SHORT libty_pl.61401F00 ; continues on as last CMP set to 1
61401ED7   F7C7 04000000    TEST EDI,4 ; EDI is not 4 (its a memory pointer)
61401EDD   74 21            JE SHORT libty_pl.61401F00 ; this jump happens

Which jumps to
61401F00   89D1             MOV ECX,EDX ; moves FFFFFFFF into ECX

61401F09   F3:A5            REP MOVS DWORD PTR ES:[EDI],DWORD PTR DS> ; memcpy

Here is a look at the registers at the time of the crash.

Registers at time of crash
EAX 00004A00
ECX 3FFF16CA
EDX FFFFFFFF
EBX 0487AEE8
ESP 061BFA64
EBP 047F7CE0
ESI 061FA000
EDI 048B53C4
EIP 61401F09 libty_pl.61401F09

The access violation occurs during the course of the memcpy call (specifically during the REP MOVS instruction). The violation references the memory address stored in ESI. This is evidence of the heap overflow!

61401F09   F3:A5            REP MOVS DWORD PTR ES:[EDI],DWORD PTR DS> ; memcpy

Patching the Code

The issue with the heap overflow described above is that you can have a memcpy that attempts to copy data of size i_map_size (which can be arbitrarily set and very large) into a small buffer. The only validation of i_map_size is done via the if condition [11] which checks to see if it is greater than 8. What this doesn’t consider is whether i_map_size is some value 0 or smaller (even up to FFFFFFFFh!). Implementing more robust validation of i_map_size to ensure it can only be a value between 1 and 8 is one way to mitigate the vulnerability.

So if we changed …

if (i_map_size > 8) {

to…

if (i_map_size < 1 || i_map_size > 8) {

then this could solve the issue!

Source Code

static void parse_master(demux_t *p_demux)
{
    demux_sys_t *p_sys = p_demux->p_sys;
    uint8_t mst_buf[32]; [1]
    int i, i_map_size; [2]
    int64_t i_save_pos = stream_Tell(p_demux->s);
    int64_t i_pts_secs;

    /* Note that the entries in the SEQ table in the stream may have
       different sizes depending on the bits per entry.  We store them
       all in the same size structure, so we have to parse them out one
       by one.  If we had a dynamic structure, we could simply read the
       entire table directly from the stream into memory in place. */

    /* clear the SEQ table */
    free(p_sys->seq_table);

    /* parse header info */
    stream_Read(p_demux->s, mst_buf, 32);	[3]
    i_map_size = U32_AT(&mst_buf[20]);  /* size of bitmask, in bytes */	[4]
    p_sys->i_bits_per_seq_entry = i_map_size * 8;
    i = U32_AT(&mst_buf[28]);   /* size of SEQ table, in bytes */	[6]
    p_sys->i_seq_table_size = i / (8 + i_map_size);	[7]

    /* parse all the entries */
    p_sys->seq_table = malloc(p_sys->i_seq_table_size * sizeof(ty_seq_table_t)); [8]
    for (i=0; i<p_sys->i_seq_table_size; i++) { [9]
        stream_Read(p_demux->s, mst_buf, 8 + i_map_size); [10]
        p_sys->seq_table[i].l_timestamp = U64_AT(&mst_buf[0]);
        if (i_map_size > 8) {	[11]
            msg_Err(p_demux, "Unsupported SEQ bitmap size in master chunk");
            memset(p_sys->seq_table[i].chunk_bitmask, i_map_size, 0);
        } else {
            memcpy(p_sys->seq_table[i].chunk_bitmask, &mst_buf[8], i_map_size);	[12]
        }
    }

================= CODE BREAK =====================

#define TIVO_PES_FILEID   ( 0xf5467abd ) [13]

================= CODE BREAK =====================

/* check if it's a PART Header */
    if( U32_AT( &p_peek[ 0 ] ) == TIVO_PES_FILEID )	[14]
    {
        /* parse master chunk */
        parse_master(p_demux); [15]
        return get_chunk_header(p_demux);
    }