Re: strcpy versus strncpy

Daniel Reed (djr@NARNIA.N.ML.ORG)
Mon, 02 Mar 1998 21:46:36 -0500

On Tue, 3 Mar 1998, Morten Welinder wrote:
) A recent article on BugTraq suggested that using strcpy should
) almost always be considered a bug. That's not right. It is,
) in fact, the wrong way around: strncpy is almost always a bug.
)
) True, strncpy will avoid buffer overruns, but that only proven
) that strncpy is better than incorrect use of strcpy. The problem
) is that such use of strncpy can introduce problems of its own:
[...]
) With dynamic allocating available, there really is no execuse for
) using strncpy, with the possible exception where memory attacks
) might be a larger problem, but that should not be the case with
) argv/environ based strings.
Recently on the vcpp@narnia.mhv.net mailing list someone asked about
creating their own function similar to printf(), to which I gave the
minimal example:

#include <stdarg.h> // for the va_* stuff
#include <stdio.h> // for fputs() and stdout
#include <string.h> // for bzero()

#define BUFSIZE 1024 // how big is our internal buffer
#define BUFLEN (BUFSIZE-1) // how many bytes of our internal buffer
// can we use (BUFSIZE - 1 for the NULL
// terminator)

char *MySprintf(char *format, ...) {
static char buf[BUFSIZE];
// this is our buffer to store the
// formatted string into
va_list msg; // this is how we access the ...

bzero(buf, BUFSIZE); // clear out our buffer
va_start(msg, format); // then attach the ... with the format
vsnprintf(buf, BUFLEN, format, msg);
// then stick the formatted string into
// buf (but only use BUFLEN bytes of buf,
// to avoid a buffer overflow)
va_end(msg); // clean up
return(buf); // send back a pointer to our static buffer
}

I made a note that this wasn't multi-thread safe, as calling MySprintf()
again would overwrite the static buffer for MySprintf().

If I had made this use dynamic memory, instead of a static internal
buffer, the user would then have to deal with free()'ing a section of
memory they did not allocate--my function did! If nothing else, that's
poor coding style (in my opinion), and at worse, leads to hard-to-trace
memory leaks.

So, discounting dynamic memory allocation, could you fault me for shunning
vsprintf() and instead using vsnprintf()?

...

int main(int argc, char **args) {
printf("Shell login wrapper loading.\n");
printf("\n");
if (getuid() == 0)
execl(MySprintf("%s.root", getenv("SHELL")), args[0], NULL);
/* set user limits and stuff */
execl(MySprintf("%s.user", getenv("SHELL")), args[0], NULL);
}

as a trivial example. As a not so trivial example, think of something like
sendmail, which runs forever, and uses a lot of automatic buffers. Running
as root, having a static buffer and using strcpy/sprintf/vsprintf, buffer
overflows are possibly exploitable. As any user, having a dynamic buffer
and using anything, memory starvation (or CPU starvation, in fact;
malloc() is an expensive call) is possible. Under any user, using a static
buffer with strncpy/snprintf/vsnprintf, buffer overflows are significantly
reduced (if not eliminated), resource starvation is significantly reduced
(if not eliminated), and at worse an incoming, legitimate message will be
bounced because it overflows a buffer. I believe in [one of] the SMTP
RFC[s] a maximum line length is defined for commands.

--
Daniel Reed <n@narnia.n.ml.org> (3CE060DD)
System administrator of narnia.n.ml.org (narnia.mhv.net [199.0.0.118])
I'm so glad to see you! I've run out of people to torment...