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 (15)

  1. Mike Dunn says:

    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 ๐Ÿ˜‰

  2. Carlos says:

    In addition:

    #3: pszDest and szDest are confused.

    #6: MAX is not defined.

  3. Michael Howard says:

    Re #3 and #6 – code tweaked, thanks!

  4. The Quiz #3 Should be like this?

    // Example #3

    #define MAX (50)

    char szDest[MAX];

    strncpy(szDest,pszSrc,MAX)

    szDest[MAX] = ‘’;

  5. Centaur says:

    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.

  6. Lorad says:

    // 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.

  7. anonymous says:

    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.

  8. S. Bala says:

    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.

  9. John C. Kirk says:

    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.)

  10. Michael Howard says:

    >>riddle me this

    what about loops?

  11. Carlos says:

    What was the answer to #6?

  12. Bryan says:

    >>>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.

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