While being busy studying the byacc source code (expect more on this soon), I came across an old list that I once assembled. It was a list of 10 very common C programming pitfalls that, when exploited, may lead to arbitrary code execution. I decided to publish it here in order aid my blog readers identify bugs in C code more easily. So here’s my list. It’s by no means complete, feel free to contact me if you want to contribute. So, after checking for trivial signedness bugs, null pointer dereferences, simple stack & heap overflows etc make sure that you also check the following list. Notice that most of the cases presented are real examples found in commonly used open source software.
Case 1: Making use of snprintf()’s return value
Many programmers use the value returned by snprintf() in order to calculate the next free position in a buffer. Consider the following example. Our imaginary programmer tried to copy two user controlled buffers inside another. He also used snprintf() for security purposes since strcat()/strcpy() are considered dangerous.
int pos; /* ... */ pos = snprintf(buffer, size, "%s", user_controlled_buffer1); size -= pos; pos = snprintf(buffer + pos, size, "%s", user_controlled_buffer2);
This is a very common mistake. According to snprintf()’s manual page, the returned value may exceed the size of the target buffer indicating that more space is needed for the user data to fit in the destination. If the first call to snprintf() returns a position greater than sizeof(buffer), then the second snprintf() will attemp to write data outside the target buffer’s boundaries. Additionally, size -= pos may result in a negative result which in turn may lead to other problems.
Case 2: Buffer increase on demand
This is actually similar to case 1 but it doesn’t lead to directly exploitable conditions. Consider a program that describes I/O buffers using structures. For example, one such structure may contain the actual data as well as an integer indicating the data length.
typedef struct {
char *data;
int len;
} io_t;
I’ve encountered a bunch of applications doing stuff like this:
io_t *whatever; /* ... */ whatever->len += snprintf(whatever->data, size, "%s", user_controlled_buffer);
The variable whatever->len may eventually receive a value greater than the real size of the data region. Although this is not directly exploitable, it usually leads to exploitable conditions.
Case 3: Using strncpy() safely
Ok this is probably the most common mistake. Calling strncpy() like this…
strncpy(buffer, user_controlled_buffer, sizeof(buffer));
…is kinda nasty since it may result in off-by-one errors. On the contrary…
strncpy(buffer, user_controlled_buffer, sizeof(buffer) - 1);
…is much safer. No need to discuss this further since many public exploits target such a vulnerebility and there are plenty of resources on this matter. I still wonder why I included strncpy() in this list!
Case 4: realloc() frees the original chunk on success but not on failure
Here’s another very common C snippet.
new = realloc(old, new_size); if(!new) return;
This is definitively a memory leak, since the call to realloc() won’t free the old chunk in case it fails. This problem alone is not sufficient to cause an exploitable condition, yet, it is a bless for all those people who do code heap exploits. By properly forcing the target program to leak memory, an attacker may setup the heap the way they like.
Case 5: open() race conditions
Most programs need to perform I/O on a file or device. For security purposes, they usually perform various checks first e.g if the file belongs to root, if it is world writable and so on. If the sanity tests are successfuly passed, they continue by actually opening the target file using open() or some similar function. This way of opening files allows for the target file or device to be modified within a time window (i.e after the checks have taken place but before it is actually opened). The AUCERT security checklist, which used to be here, was a neat source of information on how to avoid race conditions. Unfortunately, the link is now dead.
AUCERT proposed that one should check the inode of the target file before and after it is opened. If the inodes do not match then this is probably an indication of a symlink attack or a race condition. I promise I’ll post secure_open.c here when I have some free time to actually implement it :-)
Case 6: Pattern matching is more or less dangerous
Some months ago, a friend of mine was trying to bypass the registry checks performed by a very famous and widely used antivirus suite for Microsoft Windows. His code called RegOpenKey() on a registry location which was considered a security threat by the AV rules. The first thing we actually tested was to slightly obfuscate the registry path by adding extra slashes in the path.
\\\\\\\\\\path\\\\\\\\to\\\\\\\\registry\\\\\\\\key
And guess what? It actually worked! Never underestimate a stupid idea!
Case 7: Using unions safely
I first noticed this one in Dovecot’s secure coding guide. If you haven’t read it yet, then you should do it now. This little text file states that mixing integer and pointer members in unions may result in serious problems that can be easily exploited to achieve arbitrary code execution as well as other fancy stuff :-)
Case 8: Authentication via environmental variables
I was quite surprised to see that the external authentication mechanism used by pureftpd makes use of environmental variables. More precisely, after receiving the credentials from the user, pureftpd exports the given username and password in a pair of environment variables. Then, pureftpd calls the authentication backend which, in turn, decides if authentication is successful. OpenBSD (and possibly others?) implements the kvm_getproc2() and kvm_getenvv2() functions, which allow a non-root user to read the environment of another (possibly privileged) process. There exists a time window (starting before the execve() of the authentication backend and ending after the backend calls unsetenv()) during which a non-root user can sniff the usernames and passwords sent to the ftp server. The following code demonstrates this technique. There’s at least one more widely used open source server that uses this kind of authentication… be careful!
Case 9: Be careful when using free() in for() loops
Invalid usage of free() in for() loops may result in double frees or in invalid memory being accessed. Here’s a very cool example which can be found in K&R page 167.
for(p = head; p != NULL; p = p->next) /* This is wrong! */ free(p);
Notice that since the pointer ‘p’ is freed via a call to free(), it is not legal to use p = p->next in the for() loop because ‘p’ is not guaranteed to point to a valid memory. The correct way of freeing a list of items is the following:
for (p = head; p != NULL; p = q) {
q = p->next;
free(p);
}
Case 10: Null termination tricks
Last but not least, I’ve come across several applications doing the following in order to NULL terminate a buffer.
strncpy(buffer, user_controlled_buffer, sizeof(buffer) - 1); buffer[strlen(buffer) - 1] = 0;
This is a very dangerous practice. Notice that an empty user_controlled_buffer[] can result in a null byte landing on buffer[-1]. This, in turn, may result in unexpected behavior and possibly exploitable conditions. Generally speaking, any code of the form…
buffer[len - 1] = 0;
…is very dangerous when ‘len’ is tainted ;-)
– dp
