Bug 33207

Summary: Results of my suexec.c code audit
Product: Apache httpd-2 Reporter: Roland Illig <roland.illig>
Component: supportAssignee: Apache HTTPD Bugs Mailing List <bugs>
Status: NEW ---    
Severity: normal Keywords: PatchAvailable
Priority: P3    
Version: 2.0.52   
Target Milestone: ---   
Hardware: All   
OS: All   

Description Roland Illig 2005-01-23 13:41:31 UTC
I've done a code audit of the suexec.c file just for fun.
Here are my results:

* you will have problems when sizeof(uid_t) > sizeof(int).
  (consider 64 bit uid_t and 32 bit int; therefore don't use atoi(3).)

* you will have problems when sizeof(gid_t) > sizeof(int).
  (consider 64 bit gid_t and 32 bit int; therefore don't use atoi(3).)

* you did not check for NULL returns of strdup.
  Fixed in the patch.

* you had a possible ("compile time") buffer overflow.
  As long as AP_SAFE_PATH is a string literal (which it should really be),
  the patched version is faster and occupied less memory.
  Fixed in the patch.

* you had an unused variable (main:prog) in the code.
  Fixed in the patch.

* you are still not able to report the failure of execv to the logfile.
  See the other bug reports.

Index: suexec.c
===================================================================
--- suexec.c	(revision 126209)
+++ suexec.c	(working copy)
@@ -200,9 +200,19 @@
     return;
 }
 
+static char *xstrdup(const char *s)
+{
+    char *result;
+
+    if ( (result = strdup(s)) == NULL) {
+        log_err("out of memory\n");
+        exit(1);
+    }
+    return result;
+}
+
 static void clean_env(void)
 {
-    char pathbuf[512];
     char **cleanenv;
     char **ep;
     int cidx = 0;
@@ -224,8 +234,7 @@
         exit(120);
     }
 
-    sprintf(pathbuf, "PATH=%s", AP_SAFE_PATH);
-    cleanenv[cidx] = strdup(pathbuf);
+    cleanenv[cidx] = "PATH=" AP_SAFE_PATH;
     cidx++;
 
     for (ep = envp; *ep && cidx < AP_ENVBUF-1; ep++) {
@@ -254,7 +263,6 @@
     char *target_homedir;   /* target home directory     */
     char *actual_uname;     /* actual user name          */
     char *actual_gname;     /* actual group name         */
-    char *prog;             /* name of this program      */
     char *cmd;              /* command to be executed    */
     char cwd[AP_MAXPATH];   /* current working directory */
     char dwd[AP_MAXPATH];   /* docroot working directory */
@@ -268,7 +276,6 @@
      */
     clean_env();
 
-    prog = argv[0];
     /*
      * Check existence/validity of the UID of the user
      * running this program.  Error out if invalid.
@@ -395,11 +402,11 @@
             exit(106);
         }
         gid = gr->gr_gid;
-        actual_gname = strdup(gr->gr_name);
+        actual_gname = xstrdup(gr->gr_name);
     }
     else {
         gid = atoi(target_gname);
-        actual_gname = strdup(target_gname);
+        actual_gname = xstrdup(target_gname);
     }
 
 #ifdef _OSD_POSIX
@@ -433,8 +440,8 @@
      * Save these for later since initgroups will hose the struct
      */
     uid = pw->pw_uid;
-    actual_uname = strdup(pw->pw_name);
-    target_homedir = strdup(pw->pw_dir);
+    actual_uname = xstrdup(pw->pw_name);
+    target_homedir = xstrdup(pw->pw_dir);
 
     /*
      * Log the transaction here to be sure we have an open log