configure ETCDIR buffer overflow

I just had a quick look at the source for webalyser, which is an opensource web log parser, and saw an interesting buffer overflow.  Before people get excited, its not a security problem or likely to ever be encountered.  It is presented here as an example of how bugs like this are still present in non critical code.

#define BUFSIZE 4096
char tmp_buf[BUFSIZE];
sprintf(tmp_buf,"%s/webalizer.conf", ETCDIR);

This is a classic buffer overflow that occurs when ETCDIR is configured to a size greater than BUFSIZE.  ETCDIR is user configured at buildtime via the configure script.

 In addition, when I first looked at main() there were about 20 variable declarations, and a mess of spaghetti would surely be to follow.  This was going to be a long night..

 The correct solution to avoid this buffer overflow would be to use snprintf.  snprintf might truncate the result however, so a better solution would be to dynamically allocate the memory required.

length = snprintf(NULL, 0, "%s/webalyser.conf", ETCDIR);
tmp_buf = malloc(length + 1);
if (tmp_buf == NULL) error();
snprintf(tmp_buf, length, "%s/webalyser.conf", ECTDIR);

The last snprintf could have been replaced with sprintf also, but I chose to use snprintf (read below).

The code above relies upon the behaviour of snprintf accepting NULL instead of a string argument and returning the number of bytes, excluding the trailing NUL, that would have been written to the string.  This behaviour is present from Linux glibc 2.1.  If you don’t have a system that implements this behaviour in snprintf, then you will be required to write a more complicated version of the code.

length = strlen(ETCDIR) + strlen("/webalyser.conf") + 1;
tmp_buf = malloc(length);
if (tmp_buf == NULL) error();
snprintf(tmp_buf, length, "%s/webalyser.conf", ECTDIR);

The reason I chose to use snprintf and not sprintf, is because I believe that sprintf is inherently unsafe, and should not be used AT ALL – even when you know how to use it.  People might disagree with that view.

 A minor improvement might be to replace those strlen’s with sizeof.

length = sizeof(ETCDIR) + sizeof("/webalyser.conf") + 1;

In conclusion, I think many non system or performance critical programs written in legacy C have past their use by date.  If a modern language was used, be it either a scripting language or Java, perhaps even C#, then buffer overflows would be impossible to implement, and code readability generally improved.

2 responses to “configure ETCDIR buffer overflow

  1. Hey Silvio,

    What do you think about using asprintf as an alternative to the examples you have given above? I’m thinking asprintf might be considered more friendly, as theres less chance a developer could make a mistake. In your code:

    length = snprintf(NULL, 0, “%s/webalyser.conf”, ETCDIR);
    tmp_buf = malloc(length + 1);
    if (tmp_buf == NULL) error();
    snprintf(tmp_buf, length, “%s/webalyser.conf”, ECTDIR);

    Is it ever possible to make ETCDIR/webanalyser.conf so large, that length + 1 wraps and subsequently the snprintf will overflow? I have tried writing POC to make strlen() wrap in the past, but with no luck.. (I always end up with an out of memory condition). Im wondering if you think its a practically exploitable situation? Because of that type of mistake I was thinking asprintf might be more friendly

  2. oh i forgot to mention, it looks possible to make length = -1 if the format string is NULL, im not sure under what conditions that’d be exploitable though unless the second snprintf is different

Leave a Reply

Fill in your details below or click an icon to log in: Logo

You are commenting using your account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s