CODEWITHSUNDEEP
csecuritybuffer-overflow
rakagunarto
rakagunarto

Reputation: 315

Is this C user input code vulnerable?

I have this code that reads input from the user:

unsigned int readInput(char * buffer, unsigned int len){
    size_t stringlen = 0;
    char c;
    while((c = getchar()) != '\n' && c != EOF){
        if(stringlen < (len-1)){
            buffer[stringlen] = c;
            stringlen++;
        }
    }
    buffer[stringlen+1] = '\x00';
    return stringlen;
}

The size of char * buff is already set to len and has been memset to contain "0"s. Is this code vulnerable to any vulnerability attacks?

Upvotes: 0

Views: 2257

Answers (4)

kozel
kozel

Reputation: 207

Your question is whether or not the code is vulnerable to attacks. The answer is no, it is not vulnerable, certainly not by the common definitions of vulnerability.

This is an interface between some unknown input (possibly by an adversary) and a buffer. You have correctly included a mechanism that prevents a buffer overflow, so your code is safe. [We assume here that everything from getchar() down is not subject of your question].

Whether the code will work as intended is a different story, (others already pointed out the hole before the terminating NULL), but that was not your question.

Upvotes: -1

TheGreatContini
TheGreatContini

Reputation: 6629

Assuming the buffer is allocated len bytes, the most glaring problem is:

buffer[stringlen+1] = '\x00';

This is because the loop can exit with stringlen equal to len-1, and therefore you are writing to buffer[len]. However, you should only be writing to indices up to len-1.

So let's fix this as follows:

buffer[stringlen] = '\x00';

This is what you really want because you have not written to buffer[stringlen] yet.

A subtler error is that if len is 0 (which you probably would say should never happen), then len-1 is MAXINT and hence (stringlen < (len-1)) is always true. Thus, the code will always buffer overflow on a 0 length buffer.

Upvotes: 1

Kerrek SB
Kerrek SB

Reputation: 477338

Your code potentially writes out of bounds and leaves an array element uninitialized:

char buf[10];  // uninitialized!

readInput(buf, 10);   // feed 12 TB of data

This has undefined behaviour because you write to buf[10].

readInput(buf, 10);  // feed 8 bytes of data

strlen(buf);

This has undefined behaviour because you read the uninitialized value buf[8].

The error lies in the way you assign the null terminator, which uses the wrong index. It should say:

buffer[stringlen] = '\0';
//     ^^^^^^^^^

Because you compute len - 1, your code should also have a precondition that len must be strictly positive. This is sensible, because you promise to produce a null-terminated string.

Upvotes: 1

unwind
unwind

Reputation: 399949

Depending on the platform, unsigned int might be too small to hold the number 13194139533312. You should always use size_t for buffer sizes in C, not doing so might be a vulnerability, yes.

Also, of course getchar() doesn't return char, so that's broken too.

I'd say "yes", that code is vulnerable.

Upvotes: 1

Related Questions