possible buffer overflow vulnerability for va_list in C?

Question

I have the following code:

int ircsocket_print(char *message, ...)
{
    char buffer[512];
    int iError;
    va_list va;
    va_start(va, message);
    vsprintf(buffer, message, va);
    va_end(va);
    send(ircsocket_connection, buffer, strlen(buffer), 0);
    return 1;
}

And I wanted to know if this code is vulerable to buffer overflows by providing char arrays with a size > 512 to the variables list? And if so - How can I fix this?

thank you.

Answer 1

Yes, it is vulnerable.

You can implement your function this way:

int ircsocket_print(char *message, ...)
{
    char buf[512];
    char *buffer;
    int len;
    va_list va;

    buffer = buf;
    va_start(va, message);
    len = vsnprintf(buffer, 512, message, va);
    va_end(va);

    if (len >= 512)
    {
        buffer = (char*)malloc(len + 1);
        va_start(va, message);
        len = vsnprintf(buffer, len + 1, message, va);
        va_end(va);
    }

    send(ircsocket_connection, buffer, len, 0);

    if (buffer != buf)
        free(buffer);
    return 1;
}

Answer 2

Yes, it is vulnerable.

Simply use vsnprintf instead:

vsnprintf(buffer, sizeof(buffer), message, va);

Answer 3

As everyone else noted, the basic answer to your question is "Yes, you are vulnerable to buffer overflows".

In C99, you could use a VLA:

void ircsocket_print(const char *fmt, ...)
{
    va_list args;

    va_start(args, fmt);
    int len = vsnprintf(0, 0, message, args);
    va_end(args);

    char buffer[len+1];

    va_start(args, fmt);
    int len = vsnprintf(buffer, len+1, message, args);
    va_end(args);

    send(ircsocket_connection, buffer, len, 0);
}

Note the idiom that calls vsnprintf() once with length 0 (the second zero) to obtain the length required, then calls it a second time to format the data into the buffer. Also note the careful resetting of args after each call to vsnprintf(); that is required by the C standard:

§7.15 <stdarg.h>

If access to the varying arguments is desired, the called function shall declare an object (generally referred to as ap in this subclause) having type va_list. The object ap may be passed as an argument to another function; if that function invokes the va_arg macro with parameter ap, the value of ap in the calling function is indeterminate and shall be passed to the va_end macro prior to any further reference to ap.

One downside to this formulation is that it takes a pessimistic view and unconditionally calls vsnprintf() twice. You might prefer to take an optimistic view (most of the time, 512 will be enough), and only allocate more space if the first call shows that it is insufficient.

One more downside to the use of a VLA like this is that if you run out of space for the local variable buffer, your code may never get a chance to recover. You must judge how serious a problem that is. If it is an issue, use explicit memory allocation (malloc()) instead:

char buffer = malloc(len+1);
if (buffer == 0)
    return;  // Report error?
...second vsnprintf() and send()...
free(buffer);

Since your function only ever returned the constant 1, there is no obvious reason to make it a function returning anything - if it is a function returning void, the calling code does not need any check on the returned value. OTOH, maybe you should be returning the result of the send() call (and possibly an error indication if the malloc() fails).

I also made the format (message) parameter into a const char *; neither this function nor the ones it calls modify the format string.

Answer 4

If you're on Linux you could use vasprintf() which will allocate a buffer of the correct size.

If you need portability you can use vsnprintf() to avoid buffer overflows.