Refactoring C: Do We Need a Security Review?

Now that I’m actually doing real work with input from the network, I thought it would be a good time to stop and take a look at whatever I’m exposing stuff. C is known for buffer overruns and security issues, and compounding that with network software that accepts untrusted input, that is something that we should take a look at.

The first line of defense is to use Valgrind and see if it reports any errors. It reported a memory leak (I didn’t free the command’s buffer, it seemed), which was easy to fix. But it also reported a much more serious issue:

Conditional jump or move depends on uninitialised value(s)

This is not something that I wanted to see. The nice thing about Valgrind is that it prints a nice stack trace, even if in this case, it didn’t make any sense. It was deep inside the strcmp() function, which I assume is fine. I dug around on how this warning in implemented before I got what was going on. Basically, I was handing strcmp memory that was never initialized, which caused this warning.

Here is the relevant piece of the code:

struct cmd* read_message(struct connection * c) {
int rc, to_read, to_scan = 0;
do
{
if (c->used_buffer > 0) {
printf("scan: %i, used: %i", to_scan, c->used_buffer);
char* final = strnstr(c->buffer + to_scan, "\r\n\r\n", c->used_buffer);
if (final != NULL) {
struct cmd* cmd = parse_command(c, c->buffer, final - c->buffer + 2/*include one \r\n*/);
c->used_buffer -= (final + 4) - c->buffer;
memmove(c->buffer, final + 4, c->used_buffer);
return cmd;
}
to_scan = c->used_buffer - 3 < 0 ? 0 : c->used_buffer - 3;
}
to_read = MSG_SIZE - c->used_buffer;
if (to_read == 0) {
push_error(EINVAL, "Message size is too large, after 8KB, "
"couldn't find \r\n separator, aborting connection.");
return NULL;
}
rc = connection_read(c, c->buffer + c->used_buffer, to_read);
if (rc == 0)
return NULL; // broken connection, probably
c->used_buffer += rc;
} while (1);
}

Take a look and see if you can see the error.

It might be helpful if I told you that this is an error that would only be raised if I’m writing to the stream manually, not via any client API.

It took me a while to figure out what was going on. This piece of code is meant to be called multiple times, building a single buffer (of up to 8KB in size) from potentially multiple network reads.

I had a small optimization there to avoid scanning from the beginning of the string and just scan from where we already scanned. This is the to_scan variable. As it turned out, this darling had nasty consequences.

Look at line 7 in the code sample, I’m telling strnstr() to start reading from the string from the specified position, but I pass the original size. I read past the end of the buffer. Likely still in my own memory, but that would almost certainly have caused issues down the road, and it is easy to construct a sequence of operations that would cause me to thing that a message is over when I haven’t finished actually sending it (reading the \r\n\r\n divider from a previous message).

Once that was fixed, it was green across the board. But I’m not a C programmer, and I’m not sure if there are other stuff that I should be looking at. I’m using string functions with explicit length, doing proper error checking, etc. Code review for this hasn’t show any issue, but I’m sure that there are more stuff there.

The actual code is about 100 lines of C code that I think is fairly straightforward. I would be very happy to hear what else I can do to this piece of code to increase my confidence in it.