Buffer Overflow Sample: Why is this code dangerous?

Question

I'm fairly new to c and I'm reading a book regarding Software Vulnerabilities and I came across this buffer overflow sample, it mentions that this can cause a buffer overflow. I am trying to determine how this is the case.

int handle_query_string(char *query_string)
{
    struct keyval *qstring_values, *ent;
    char buf[1024];
        
    if(!query_string) {
        return 0;
    }
    
    qstring_values = split_keyvalue_pairs(query_string);
        
    if((ent = find_entry(qstring_values, "mode")) != NULL) {
        sprintf(buf, "MODE=%s", ent->value);
        putenv(buf);
    }
}

I am paying close attention to this block of code because this appears to be where the buffer overflow is caused.

if((ent = find_entry(qstring_values, "mode")) != NULL)
{
    sprintf(buf, "MODE=%s", ent->value);
    putenv(buf);
}

Answer 1

A very big thing here is that you passed a pointer to a local variable to putenv. The buffer will cease to exist when handle_query_string returns. After that it will contain garbage variables. Note that what putenv does require that the string passed to it remains unchanged for the rest of the program. From the documentation for putenv (emphasis mine):

int putenv(char *string);

The putenv() function adds or changes the value of environment variables. The argument string is of the form name=value. If name does not already exist in the environment, then string is added to the environment. If name does exist, then the value of name in the environment is changed to value. The string pointed to by string becomes part of the environment, so altering the string changes the environment.

This can be corrected by using dynamic allocation. char *buf = malloc(1024) instead of char buf[1024]

Another thing is that sprintf(buf, "MODE=%s", ent->value); might overflow. That would happen if the string ent->value is too long. A solution there is to use snprintf instead.

snprintf(buf, sizeof buf, "MODE=%s", ent->value);

This prevents overflow, but it might still cause problems, because if ent->value is too big to fit in buf, then buf will for obvious reasons not contain the full string.

Here is a way to rectify both issues:

int handle_query_string(char *query_string)
{
    struct keyval *qstring_values, *ent;
    char *buf = NULL;

    if(!query_string)
        return 0;

    qstring_values = split_keyvalue_pairs(query_string);

    if((ent = find_entry(qstring_values, "mode")) != NULL)
    {
        // Make sure that the buffer is big enough instead of using
        // a fixed size. The +5 on size  is for "MODE=" and +1 is 
        // for the string terminator
        const char[] format_string = "MODE=%s";
        const size_t size = strlen(ent->value) + 5 + 1;
        buf = malloc(size);

        // Always check malloc for failure or chase nasty bugs
        if(!buf) exit(EXIT_FAILURE);    

        sprintf(buf, format_string, ent->value);
        putenv(buf);
    }
}

Since we're using malloc the allocation will remain after the function exits. And for the same reason, we make sure that the buffer is big enough beforehand, and thus, using snprintf instead of sprintf is not necessary.

Theoretically, this has a memory leak unless you use free on all strings you have allocated, but in reality, not freeing before exiting main is very rarely a problem. Might be good to know though.

It can also be good to know that even though this code now is fairly protected, it's still not thread safe. The content of query_string and thus also ent->value may be altered. Your code does not show it, but it seems highly likely that find_entry returns a pointer that points somewhere in query_string. This can of course also be solved, but it can get complicated.