An example of "buffer overflow" vulnerability:
The solution (1)

Where is the bug?

As you may have figured out, the segmentation fault occurs because the program appends either ".img" or ".img.z" to the file name with strcat, without providing space for them in the declaration of filename.

So the first correction to be made is

  old   char filename[MAXNAMELENGTH];  
  new   char filename[MAXNAMELENGTH+6];

The first moral of the exercise is:

When concatenating strings, be sure to allocate enough space for the result.

Wait, there is more...

That correction should take care of the segment overflow, but the bug is not fixed yet! If you look at the argument parsing code, you will see that it does check whether the file name is not too long:

  /* Check name length (including final '\0'): */
  if (strlen(argc[i]) > MAXNAMELENGTH+1)
However, the test is wrong! If MAXNAMELENGTH is the maximum name length including the final 0, the `+1' should have been on the left side of the `>', not on the right side! On the other hand, if MAXNAMELENGTH excludes the final 0, then there should be no `+1', and we must reserve space for that 0 explicitly in the declaration of filename.

The effect of this bug is to accept banner names with up to 9 letters, such as garotinho, even though MAXNAMELENGTH is only 8. After adding the extensions .img.z, plus the final 0, the file name grows to 15 bytes; yet, even with the fix above, the filename variable is only 14 bytes long!

Thus, the correction proposed before does not remove the buffer overflow bug: even though we haven't noticed, the final 0 byte is overwriting some other variable! To really fix the bug, and still allow garotinho as an argument, we must do the following changes:

  old #define MAXNAMELENGTH 8    
  new #define MAXNAMELENGTH 9    

  old   char filename[MAXNAMELENGTH];  
  new   char filename[MAXNAMELENGTH + 7];  

  old         /* Check name length (including final '\0'): */    
  new         /* Check name length: */     

  old         if (strlen(argc[i]) > MAXNAMELENGTH+1)  
  new         if (strlen(argc[i]) > MAXNAMELENGTH)

You should be ready now to appreciate the second moral of the exercise:

When debugging, read the code, not the comments!

Wait, we aren't done yet!

You are urged to apply the above corrections to the program, run again the last test (demo garotinho), and then (only then!) click here.

Last edited on 2010-10-17 17:02:47 by stolfi