The Evils of strncat and strncpy redux

Published 02 November 04 01:54 PM

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.

 

by michael_HOWARD
Filed under:

Comments

# Mike Dunn said on November 2, 2004 2:29 PM:
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 ;)
# Carlos said on November 2, 2004 2:59 PM:
In addition:

#3: pszDest and szDest are confused.

#6: MAX is not defined.
# Michael Howard said on November 2, 2004 3:03 PM:
Re #3 and #6 - code tweaked, thanks!
# 厚重之刀 said on November 2, 2004 9:21 PM:
The Quiz #3 Should be like this?
// Example #3
#define MAX (50)
char szDest[MAX];
strncpy(szDest,pszSrc,MAX)
szDest[MAX] = '\0';
# Centaur said on November 3, 2004 12:21 AM:
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.
# Lorad said on November 3, 2004 5:13 AM:
// Example #1 (code prior to this verifies
sizeof(pszSrc) implies it will be sizeof(void*) - this is machine dependant, but not 50

// Example #2
if pszSrc is null error
if pszSrc is longer than 50 characters the string will not be null terminated

// Example #3
pszDest[MAX] is past the end of the allocated memory

// Example #4
strncat(szDest,pszSrc,MAX-1);
Unless szDest is empty at this point or pszSrc length + szDest length is less than MAX will cause an over run. The last parameter is the number of characters to concat onto destination, not total length.

// Example #5
strlen(szDest) will be undefined, depending on what crap exists in the array at allocation time.

// Example #6
The only thing I can see here is if p is null then you have issues.


# Software Test Engineering @ Microsoft said on November 3, 2004 12:13 PM:
# anonymous said on November 3, 2004 10:32 AM:
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.

# S. Bala said on November 3, 2004 6:32 PM:
Since we're in the mood, riddle me this:

A computer CPU can process 1 million machine language instructions per second. If a program includes 1000 lines of code and each line represents an average of 10 instructions, calculate the amount of time in milliseconds for the CPU to process the entire program.


# John C. Kirk said on November 4, 2004 6:08 AM:
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.)
# Michael Howard said on November 4, 2004 3:00 PM:
>>riddle me this
what about loops?
# Carlos said on November 8, 2004 4:31 PM:
What was the answer to #6?
# Bryan said on November 22, 2004 6:51 AM:
>>>riddle me this
>what about loops?

And what about lines of code that include CALL opcodes (they call functions)? Even if we assume that the size of all functions called is included in the "10 instructions per line" or the "1000 lines of code" numbers, a lot of those instructions are very likely HLTs, which cause the processor to stop until an interrupt is generated. Likely because the program is waiting for user input, or something similar.

If the program is CPU-bound, and there are no loops, then the program will finish in .01 seconds. Otherwise, we can't say much of anything about its runtime.
# Michael Howard's Web Log said on October 30, 2006 1:07 PM:

From: The Learning from Mistakes Dept. A few months back eEye found an exploitable buffer overrun in

New Comments to this post are disabled
Page view tracker