Tito writes:

Hi,
I've spent the half night staring at the devilish  my_getpwuid and my_getgrgid functions
trying to find out a way to avoid actual and future potential buffer overflow problems
without breaking existing code.
Finally I've  found a not intrusive way to do this that surely doesn't break existing code
and fixes a couple of problems too.
The attached patch:
1) changes the behaviour of my_getpwuid and my_getgrgid to avoid potetntial buffer overflows
2) fixes all occurences of this function calls in tar.c , id.c , ls.c, whoami.c, logger.c, libbb.h.
3) The behaviour of tar, ls and  logger is unchanged.
4) The behavior of ps with somewhat longer usernames messing up output is fixed.
5) The only bigger change was the increasing of size of the buffers in id.c to avoid
     false negatives (unknown user: xxxxxx) with usernames longer than 8 chars.
     The value i used ( 32 chars ) was taken from the tar header ( see gname and uname).
     Maybe this buffers can be reduced a bit  ( to 16 or whatever ), this is up to you.
6) The increase of size of the binary is not so dramatic:
     size busybox
       text    data     bss     dec     hex filename
     239568    2300   36816  278684   4409c busybox
    size busybox_fixed
       text    data     bss     dec     hex filename
     239616    2300   36816  278732   440cc busybox
7) The behaviour of whoami changed:
    actually it  prints out an username cut down to the size of the buffer.
    This could be fixed by increasing the size of the buffer as in id.c or
    avoid the use of my_getpwuid and use getpwuid directly instead.
    Maybe this colud be also remain unchanged......

Please apply if you think it is ok to do so.
The diff applies on today's cvs tarball (2004-08-25).
Thanks in advance,
Ciao,
Tito

diff --git a/archival/tar.c b/archival/tar.c
index 689dd14249056df93e39ab02b287cf8c357069bf..950e21dd3a8298a7bd20585d0b313ed63355b21b 100644 (file)
--- a/archival/tar.c
+++ b/archival/tar.c
@@ -234,9 +234,9 @@ static inline int writeTarHeader(struct TarBallInfo *tbInfo,
                        TAR_MAGIC_LEN + TAR_VERSION_LEN);
 
        /* Enter the user and group names (default to root if it fails) */
-       if (my_getpwuid(header.uname, statbuf->st_uid) == NULL)
+       if (my_getpwuid(header.uname, statbuf->st_uid, sizeof(header.uname)) == NULL)
                strcpy(header.uname, "root");
-       if (my_getgrgid(header.gname, statbuf->st_gid) == NULL)
+       if (my_getgrgid(header.gname, statbuf->st_gid, sizeof(header.gname)) == NULL)
                strcpy(header.gname, "root");
 
        if (tbInfo->hlInfo) {

diff --git a/coreutils/id.c b/coreutils/id.c
index 602b26ec38fdc573dbdbd47465507bcc477e7440..db8afc58575a3493e7199786aee7878715627957 100644 (file)
--- a/coreutils/id.c
+++ b/coreutils/id.c
@@ -40,7 +40,7 @@
 
 extern int id_main(int argc, char **argv)
 {
-       char user[9], group[9];
+       char user[32], group[32];
        long pwnam, grnam;
        int uid, gid;
        int flags;
@@ -64,12 +64,12 @@ extern int id_main(int argc, char **argv)
                        uid = geteuid();
                        gid = getegid();
                }
-               my_getpwuid(user, uid);
+               my_getpwuid(user, uid, sizeof(user));
        } else {
                safe_strncpy(user, argv[optind], sizeof(user));
            gid = my_getpwnamegid(user);
        }
-       my_getgrgid(group, gid);
+       my_getgrgid(group, gid, sizeof(group));
 
        pwnam=my_getpwnam(user);
        grnam=my_getgrnam(group);

diff --git a/coreutils/ls.c b/coreutils/ls.c
index a87f0ec2d609c3ced0658527f8283cd0cacf2e57..22685bcd9560d8a115bb107dc45b3ea700b77324 100644 (file)
--- a/coreutils/ls.c
+++ b/coreutils/ls.c
@@ -683,9 +683,9 @@ static int list_single(struct dnode *dn)
                        break;
                case LIST_ID_NAME:
 #ifdef CONFIG_FEATURE_LS_USERNAME
-                       my_getpwuid(scratch, dn->dstat.st_uid);
+                       my_getpwuid(scratch, dn->dstat.st_uid, sizeof(scratch));
                        printf("%-8.8s ", scratch);
-                       my_getgrgid(scratch, dn->dstat.st_gid);
+                       my_getgrgid(scratch, dn->dstat.st_gid, sizeof(scratch));
                        printf("%-8.8s", scratch);
                        column += 17;
                        break;

diff --git a/coreutils/whoami.c b/coreutils/whoami.c
index f93034d3a3b860c8b8863cfa17303ee9a490e937..e2a03b1e91c5748afde4a969ae8446a3566f9245 100644 (file)
--- a/coreutils/whoami.c
+++ b/coreutils/whoami.c
@@ -36,7 +36,7 @@ extern int whoami_main(int argc, char **argv)
                bb_show_usage();
 
        uid = geteuid();
-       if (my_getpwuid(user, uid)) {
+       if (my_getpwuid(user, uid, sizeof(user))) {
                puts(user);
                bb_fflush_stdout_and_exit(EXIT_SUCCESS);
        }

diff --git a/include/libbb.h b/include/libbb.h
index afbd0203e7c53f0d7a4cc0d1269cbbeb096349db..78b9711e82f4772b64a5bcae609c34bbc1a5d044 100644 (file)
--- a/include/libbb.h
+++ b/include/libbb.h
@@ -230,8 +230,8 @@ extern unsigned long bb_xparse_number(const char *numstr,
  * increases target size and is often not needed embedded systems.  */
 extern long my_getpwnam(const char *name);
 extern long my_getgrnam(const char *name);
-extern char * my_getpwuid(char *name, long uid);
-extern char * my_getgrgid(char *group, long gid);
+extern char * my_getpwuid(char *name, long uid, int bufsize);
+extern char * my_getgrgid(char *group, long gid, int bufsize);
 extern long my_getpwnamegid(const char *name);
 extern char *bb_askpass(int timeout, const char * prompt);
 

diff --git a/libbb/my_getgrgid.c b/libbb/my_getgrgid.c
index 907a47486697effe31e36874be12ed52fe8939f9..e6b87768773b121326c5ed22d4f98e62616cab01 100644 (file)
--- a/libbb/my_getgrgid.c
+++ b/libbb/my_getgrgid.c
@@ -27,16 +27,16 @@
 
 
 /* gets a groupname given a gid */
-char * my_getgrgid(char *group, long gid)
+char * my_getgrgid(char *group, long gid, int bufsize)
 {
        struct group *mygroup;
 
        mygroup  = getgrgid(gid);
        if (mygroup==NULL) {
-               sprintf(group, "%ld", gid);
+               snprintf(group, bufsize, "%ld", gid);
                return NULL;
        } else {
-               return strcpy(group, mygroup->gr_name);
+               return safe_strncpy(group, mygroup->gr_name, bufsize);
        }
 }
 

diff --git a/libbb/my_getpwuid.c b/libbb/my_getpwuid.c
index 21a037f75c1273e448e1dbf757efb13962131225..53f6c77eef45d359a841080d2fb8deee8fc20e70 100644 (file)
--- a/libbb/my_getpwuid.c
+++ b/libbb/my_getpwuid.c
@@ -28,16 +28,16 @@
 
 
 /* gets a username given a uid */
-char * my_getpwuid(char *name, long uid)
+char * my_getpwuid(char *name, long uid, int bufsize)
 {
        struct passwd *myuser;
 
        myuser  = getpwuid(uid);
        if (myuser==NULL) {
-               sprintf(name, "%ld", (long)uid);
+               snprintf(name, bufsize, "%ld", (long)uid);
                return NULL;
        } else {
-               return strcpy(name, myuser->pw_name);
+               return safe_strncpy(name, myuser->pw_name, bufsize);
        }
 }
 

diff --git a/libbb/procps.c b/libbb/procps.c
index 46e982766d3374b36e17868477caa32a8b9dfe16..e405fb7efcc1ce5f3efd03b2701b11cfecdce610 100644 (file)
--- a/libbb/procps.c
+++ b/libbb/procps.c
@@ -57,7 +57,7 @@ extern procps_status_t * procps_scan(int save_user_arg0
                sprintf(status, "/proc/%d", pid);
                if(stat(status, &sb))
                        continue;
-               my_getpwuid(curstatus.user, sb.st_uid);
+               my_getpwuid(curstatus.user, sb.st_uid, sizeof(curstatus.user));
 
                sprintf(status, "/proc/%d/stat", pid);
                if((fp = fopen(status, "r")) == NULL)

diff --git a/sysklogd/logger.c b/sysklogd/logger.c
index 981cef322ef84b5b7c7cd77659c6078025987768..16155316fbd04f66f8da7033b09d6c16561ed5a2 100644 (file)
--- a/sysklogd/logger.c
+++ b/sysklogd/logger.c
@@ -108,7 +108,7 @@ extern int logger_main(int argc, char **argv)
        char buf[1024], name[128];
 
        /* Fill out the name string early (may be overwritten later) */
-       my_getpwuid(name, geteuid());
+       my_getpwuid(name, geteuid(), sizeof(name));
 
        /* Parse any options */
        while ((opt = getopt(argc, argv, "p:st:")) > 0) {