[BusyBox] [SECURITY] potential buffer overflows caused by my_getgrgid() and my_getpwuid

Tito farmatito at tiscali.it
Mon Aug 23 22:19:06 UTC 2004


Hi Erik,
Hi to all,
I just want to expose a problem I've found about the use of my_getgrgid and my_getpwuid in libbb.
These two functions potentially  could cause buffer overflows or other troubles.
Two examples are:
1) the use of my_getpwuid() in procps_scan() function which affects at least the ps applet.
    A quick demonstration is:
#adduser pppppppppppppppppp (18 chars, less will do it also) 
#su  pppppppppppppppppp
#e3
 in another term run ./busybox ps
 3547 postfix    1136 S   pickup -l -t fifo -u -c -o content_filter  -o receive_override
 3548 postfix    1168 S   qmgr -l -t fifo -u -c
 3549 postfix    1516 S   tlsmgr -l -t fifo -u -c
 3554 root       1008 S   su pppppppppppppppppp
 -->3555 pppppppppS  pppp   1564 S  pppp bash
--> 3588 pppppppppS  pppp      32 S  pppp  e3

This is due to the fact that the data are copied to char user[9] in

typedef struct {
	int pid;
	char user[9];
	char state[4];
	unsigned long rss;
	int ppid;
#ifdef FEATURE_CPU_USAGE_PERCENTAGE
	unsigned pcpu;
	unsigned long stime, utime;
#endif
	char *cmd;

	/* basename of executable file in call to exec(2),
		size from kernel headers */
	char short_cmd[16];
} procps_status_t;

but my_getpwuid doesn't know the size of the buffer and passes on the whole data.

2)Another example with the id applet, where this is causing a bad behaviour rather than an overflow
with the real id we get:
#id pppppppppppppppppp
uid=502(pppppppppppppppppp) gid=504(pppppppppppppppppp) groups=504(pppppppppppppppppp)
with busybox
#./busybox id pppppppppppppppppp
id: unknown user name: pppppppp

The cause of this problems is in the use of  the strcpy function, but also sprintf maybe could cause an oveflow:

char * my_getpwuid(char *name, long uid)
{
	struct passwd *myuser;

	myuser  = getpwuid(uid);
	if (myuser==NULL) {
		/* maybe this can oveflow too ? */
		sprintf(name, "%ld", (long)uid);
		return NULL;
	} else {
		/* Here is the trouble */
		return strcpy(name, myuser->pw_name);
	}
}

Same thing for my_getgrgid

char * my_getgrgid(char *group, long gid)
{
	struct group *mygroup;

	mygroup  = getgrgid(gid);
	if (mygroup==NULL) {
		sprintf(group, "%ld", gid);
		return NULL;
	} else {
		return strcpy(group, mygroup->gr_name);
	}
}

For this one I've no demostrations of overflows yet as I'haven't looked for.
Sadly I couldn't figure out a patch so far.
Two possible solutions would be:
A) use xmalloc and strlen to allocate a big enough buffers and return a pointer to the buffer
    but this would break existing code and would need to add free() calls to avoid memory leaks.
    A difference with the original code is that the function would not return NULL upon error. 

char * my_getpwuid( long uid)
{
	struct passwd *myuser;
	char *name=NULL;

	myuser  = getpwuid(uid);
	if (myuser==NULL) {
		/* maybe this can oveflow too ? */
		name=(char *)xmalloc(size big enough for the digits of a long int +1);
		sprintf(name, "%ld", (long)uid);
	} else {
		/* Here is the trouble */
		name=(char *)xmalloc(strlen(myuser->pw_name) +1);		
		sprintf(name,"%s", myuser->pw_name);
	}
	return name;
}

B)  If in busybox systems usernames and groupnames  should have a max length of 8 chars (unlike real linux systems)
	than we could use safe_strncpy.
	But there would always be the sprintf(name, "%ld", (long)uid)  problem if an 8 char buffer is not big enough.

So maybe I'm totally wrong ;-) but now it is a trouble of the gurus to look at this.

Ciao,
Tito



More information about the busybox mailing list