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.