The Evils of strncat and strncpy redux

Following my previous post about the Apache 'fix', I was asked what code examples I had showing lousy instances of strncpy and strncat.

<rant>

Many developers think that because they are using a counted string copy function the code is safe from attack. This is simply not true, you must get the buffer size right! In short, you cannot disengage your brain, just 'coz you're using xxxx-n-zzzzz!!

</rant>

Here's some code examples, can you spot the bugs?

// Example #1 (code prior to this verifies pszSrc is <= 50 chars)
#define MAX (50)
char *pszDest = malloc(sizeof(pszSrc));
strncpy(pszDest,pszSrc,MAX);

// Example #2
#define MAX (50)
char szDest[MAX];
strncpy(szDest,pszSrc,MAX);

// Example #3
#define MAX (50)
char szDest[MAX];
strncpy(szDest,pszSrc,MAX);
pszDest[MAX] = '\0';

// Example #4
#define MAX (50)
char szDest[MAX];
strncpy(szDest,pszSrc,MAX-1);
strncat(szDest,pszSrc,MAX-1);

// Example #5
char szDest[50];
_snprintf(szDest, strlen(szDest), "%s",szSrc);

// Example #6
#define MAX (50)
void func(char *p) {
char szDest[MAX];
strncpy(szDest,p,MAX);
szDest[MAX-1] = '\0';
}

I'll post the answers in a couple of days.

Comments

• Anonymous
  November 02, 2004
  I'll just post the first thing I spot in each example...
  
  #1: sizeof(pszSrc) is 4 if pszSrc is a pointer, not a staticly-allocated array.
  #2: szDest is left unterminated if strlen(pszSrc) equals MAX
  #3: Writing "szDest[MAX]" overruns the array
  #4: Misuse of the size parameter to strncat, it should be the space left, not the total space in the array.
  #5: Author of that code doesn't understand strlen ;)

• Anonymous
  November 02, 2004
  In addition:
  
  #3: pszDest and szDest are confused.
  
  #6: MAX is not defined.

• Anonymous
  November 02, 2004
  Re #3 and #6 - code tweaked, thanks!

• Anonymous
  November 02, 2004
  The Quiz #3 Should be like this?
  // Example #3
  #define MAX (50)
  char szDest[MAX];
  strncpy(szDest,pszSrc,MAX)
  szDest[MAX] = '�';

• Anonymous
  November 02, 2004
  Should the comment “(code prior to this verifies pszSrc is <= 50 chars)” be taken as “in all these examples, it is guaranteed that there exists n such that for each 0<=i<n pszSrc[i] is valid and not null, and pszSrc[n] is null”, in other words, “strlen(pszSrc) will not overrun and will return a value less than or equal to 50”?
  
  #1: pszDest might not be null-terminated after strncpy, which might later cause a read buffer overrun.
  
  #6: If p points into an array shorter than MAX and that array is not null-terminated, strncpy will read-overrun it. Good style would be to pass the size of the buffer pointed to by p.

• Anonymous
  November 02, 2004
  The comment has been removed

• Anonymous
  November 03, 2004
  WRT #6, I usually do:
  
  char a[MAX];
  a[sizeof(a)-1] = 0;
  
  rather than
  
  char a[MAX];
  a[MAX-1] = 0;
  
  ...just in case some moron (read: anyone but me) changes the definition of a down the road. Defensive coding.
  
  

• Anonymous
  November 03, 2004
  The comment has been removed

• Anonymous
  November 03, 2004
  For #1, I think the problem is that you make the buffer at pszDest the same size as pszSrc, but then you always copy over 50 characters, even if pszSrc is <50. This would then lead to a buffer overrun, by copying random bytes into the subsequent area of memory. The correct way would be to specify the length of pszSrc as the third parameter to strncpy.
  
  (My C isn't good enough to comment on the pointer size problems that other people mentioned, or the other examples - I'm a VB.NET programmer.)

• Anonymous
  November 04, 2004
  >>riddle me this
  what about loops?

• Anonymous
  November 08, 2004
  What was the answer to #6?

• Anonymous
  November 21, 2004
  The comment has been removed

• Anonymous
  October 30, 2006
  From: The Learning from Mistakes Dept. A few months back eEye found an exploitable buffer overrun in

• Anonymous
  June 08, 2009
  PingBack from http://insomniacuresite.info/story.php?id=2129