Overcoming Buffer Overflows: A real world case study

By Shawn Asmus ·

I recently performed a manual source code review of an application module written in C. The initial code base was riddled with buffer overflow vulnerabilities. There were over 1,000 instances of calls to strcpy, strcat, sprintf, gets and a few other “unsafe” functions. Needless to say, the client’s security report was quite lengthy. Their developers went back to the drawing board. Several weeks later, they delivered a refreshed code base.

I was amused to see a new function definition: pcicopy. My Spidey sense immediately started tingling. With a name like that, it had to be secure, right? Here is something similar to what they provided:

void pcicopy(char a[],char b[])
{
   int tam,x;
   tam=strlen(b);
   for(x=0;x<tam;x++)
      a[x]=b[x];
   a[x]=0x00;
}

Were they trying to fool anyone? By implementing their own version of strcpy, they again failed to consider the situation when b (the source buffer) would not be properly null-terminated. In cases like this, strlen returns undefined and unpredictable results. In addition, if array b contains more characters than is allocated to array a, a buffer overrun is still very much possible. Even though I wanted to say "nice try, but no" in the report, I stopped myself. Instead, I included further explanation of the flaw, with code samples, links to OWASP materials, and the de facto "use safe string functions such as strncpy and strncat" recommendation. All seemed right with the world, at least for a short while. Several weeks later, I received the next code release, which thankfully included fixes to most issues. Calls to gets were replaced with fgets, and no dangerous calls to strcpy and strcat could be found. Even pcicopy was reworked, and a new function called pcicat was added.

void pcicopy(char a[],char b[])
{
   int x=-1;
   do
   {
      x=x+1;
      a[x]=b[x];
   }while(b[x]!=0x00 && x<1500);
}
void pcicat(char a[],char b[])
{
   int x=-1,y=-1;
   do
   {
      x=x+1;
   }while(b[x]!=0x00 && x<1500);
   do
   {
      y=y+1;
      a[x]=b[y];
      x=x+1;
   }while(b[y]!=0x00 && x<1500 && y<1500);
}

OK, I thought, are they just messing with me now? As you can see, the developer hardcoded a length of 1500 for both functions. Unfortunately, pcicopy and pcicat are used for buffers of all sizes, from 20 to 1500 chars. This time, the report included firm recommendations that all buffer sizes should be checked and enforced by passing the size of the destination array as a parameter to the pcicopy and pcicat functions. I even included sample code that would fix the problem once and for all. However, the next round review brought even more cleverness.

void pcicopy(char dst[],char src[])
{
   dst[0]=0x00;
   if(strlen(src)<=sizeof(dst))
      strncpy(dst,src,strlen(dst);
   else
      printf("\nError Buffer\n");
}
void pcicat(char dst[],char src[])
{
   if(strlen(dst)+strlen(src)<=sizeof(dst))
      strncat(dst,(char *)&src[0],strlen(src);
   else
      printf("\nError Buffer\n");
}

Yikes! Though the developers now chose to indeed use the "safer" functions strncpy and strncat, they not only didn't fix the problem, but severely broke their code! In the case of pcicopy, strlen still doesn't account for a src string that isn't null terminated. Secondly, it calls sizeof(dst) which returns the size of the pointer to the char array, not the size of the array itself. Finally, even if strncpy is ever called (only for very short src strings), it doesn't copy anything, as strlen(dst) always returns 0 (dst[0]=0x00)! The pcicat function contains similar issues. How many rounds were we up to? At this point I lost count. Luckily, the next code update brought the following changes, which were finally an improvement.

void pcicopy(char dst[],char src[],int max_leng)
{
   dst[0]=0x00;
   if(strlen(src)<=max_leng)
      strncpy(dst,src,strlen(dst);
   else
      printf("\nError Buffer\n");
}
void pcicat(char dst[],char src[],int max_leng)
{
   if(strlen(dst)+strlen(src)<=max_leng)
      strncat(dst,(char *)&src[0],strlen(src);
   else
      printf("\nError Buffer\n");
}

Calls to both functions used sizeof(dst)-1 for the third parameter, and all arrays were either properly initialized or explicitly null-terminated. But what's this? The pcicopy still fails to copy anything to the destination array because strlen(dst) still resolves to 0. Oh, so close!

In the end, the code was fixed, but it left me wondering why it was so hard for these C developers to get it? Perhaps they weren't trained or required to unit test their code. Perhaps their non-Windows development environment or IDE was too difficult to work with. Maybe it was a language issue (they were a non-US development team). I thought my reports were clear enough when I practically said "do this and we can get on with life", but maybe not. I suppose it could have been some combination of these.

In any case, the whole experience brought a whole new appreciation for just how hard it can be to fix buffer overflow vulnerabilities. I hope you enjoyed! Shawn Asmus, CISSP, OSCP, is a member of FishNet Security’s application security team. Shawn conducts manual and tool-assisted code reviews, application penetration tests, and various other security aerobics.

shawn-asmus

Shawn Asmus

Practice Manager, Application Security, CISSP, CCSP, OSCP

Shawn Asmus is a practice manager with Optiv’s application security team. In this role he specializes in strategic and advanced AppSec program services and lends technical expertise where needed. Shawn has presented at a number of national, regional and local security seminars and conferences.