| infamous41md |
08-01-2004 01:38 PM |
Quote:
Originally posted by osvaldomarques
Gentlemen,
I'm back here, not to flame about, but to discuss some concepts. I want to discuss about the risk of buffer overflow again. I want to discuss about strcpy. After I read the first reply to my prior post oft today, I started to think if I am sooo wrong? I program C for about 15 years. strcpy is my breakfast, my lunch and my dinner. Is it a risky function? Yes! It is unsafe to cross the street. It is dangerous go out of home. It is dangerous to stay at home. To live is dangerous!
I decided to see if someone uses this function. I greped kernel sources (2.4.26). I removed all the comments, defines and references on Makefile. Result: ~1473 calls to strcpy and ~558 to strncpy.
The risk of buffer overflow is in the use of strcpy in an uncontrolled environment as in the example I did. I would check the size of the argument passed to the program before accept it. As I am programming, one of my attributions is to specify the proper size of the variable. The unsafe code would be replaced by:
Code:
#include <stdio.h>
#include <stdlib.h>
int main(int argc, char **argv)
{
FILE *fp
;
char rd_line[128]
,cmd[] = "ls -l "
,*assy_cmd
,*p1
;
if (argc != 2)
{
fprintf(stderr, "Usage: %s <directory>\n", argv[0]);
exit(EXIT_FAILURE);
}
assy_cmd = (char *) alloca(strlen(cmd) + strlen(argv[0]) + 1);
strcpy(assy_cmd, cmd);
strcat(assy_cmd, argv[1]);
if ((fp = popen(assy_cmd, "r")) == NULL)
Please forgive me for talking about this matter again but I returned to this topic for clarification purposes.
Have a nice week end!
|
couple problems here. first, alloca() is not a function one should be using to allocate dynamic memory. in the man page it says that implementations are buggy and inconsistent. and even worse, when the function in which u call alloca returns, the memory allocated gets junked. now in this case u alloc in main, so it's inconsequential, but what if you were in a function that main called and did that? that string would be worthless after the function returned. i'm not sure if that's what u intended, but that's not what alloca is intended to be used for. secondly, look closely at the line where u alloca(), you are using the length of argv[0], when u should be using argv[1]. thirdly, u haven't checked the return value of alloca(), it returns NULL on failure(actually some versions don't return NULL and some do, another reason to avoid this function). i would do this:
Code:
ptr = calloc(1, (len = strlen(cmd) + strlen(argv[1]) + 1) );
if(!ptr) errror()
snprintf(ptr, len, "%s%s", cmd, argv[1]);
in regards to the kernel and strcpy(). the calls to strcpy() don't deal with user input(hopefully), but still, i tend to avoid that func like the plague unless it's something like this:
Code:
char buf[BS]
strcpy(buf, "bla");
but even that should be avoided. what if one day u change the size of define BS, or if u use a different string instead of "bla" and forget to change the BS. better to be safe than sorry. |
