Received: (qmail 11294 invoked by uid 2012); 5 Aug 1998 18:58:38 -0000 Message-Id: <19980805185838.11293.qmail@hyperreal.org> Date: 5 Aug 1998 18:58:38 -0000 From: Jeff Stewart Reply-To: jws@purdue.edu To: apbugs@hyperreal.org Subject: Array overflow in suexec and some suggested improvements X-Send-Pr-Version: 3.2 >Number: 2790 >Category: suexec >Synopsis: Array overflow in suexec and some suggested improvements >Confidential: no >Severity: non-critical >Priority: medium >Responsible: apache >State: open >Class: sw-bug >Submitter-Id: apache >Arrival-Date: Wed Aug 5 12:00:01 PDT 1998 >Last-Modified: >Originator: jws@purdue.edu >Organization: apache >Release: 1.3.1 >Environment: Though it's not very relavent, Solaris 2.5, AIX 4.1.5, AIX 4.3 >Description: While preparing to install Apache on one of our machines, we went over the code for suexec and found one bug and saw a few things that should probably be handled more carefully. Here is a description of the bug and changes we suggest, followed by a patch we applied to suexec.c. First, there is a bug in the clean_env() routine (starting at line 212): for (ep = environ; *ep && cidx < AP_ENVBUF; ep++) { ... } sprintf(pathbuf, ...); cleanenv[cidx] = strdup(pathbuf); cleanenv[++cidx] = NULL; If there are enough "HTTP_" elements in the environment, the "PATH" and NULL are stored beyond the end of the cleanenv[] array. The for loop should be: for (ep = environ; *ep && cidx < AP_ENVBUF-2; ep++) { ... } [ in the patch, we actually store the "PATH=..." first and compare against AP_ENVBUF-1 ] While cleaning the environment, the environment should be clean. (e.g. malloc() may get the name of a file for writing debugging info. Bad news if MALLOC_DEBUG_FILE is set to /etc/passwd. Sprintf() may be susceptible to bad locale settings....) Add the following at the beginning of clean_env(): char *empty_ptr = (char *)0; char **envp; envp = environ; environ = &empty_ptr; /* VERY safe environment */ And change the for loop to: for (ep = envp; ....) { Also, in building the new environment, malformed entries can be added (e.g. no "=" or illegal variable names - sh tends to get confused by bad names). Check for valid names by adding an "=" to each string in the safe_env_list[] and check the "HTTP_" names with the following (at line 213): if (!strncmp(*ep, "HTTP_", 5) { char *cp = *ep; /* could use *ep + 5 */ while (*cp && (isalnum(*cp) || *cp == '_')) { ++cp; } if (*cp == '=') { cleanenv[cidx] = *ep; cidx++; } } When checking the current directory name against the doc_root (or userdir) name, if ((strncmp(csd, dwd, strlen(dwd))) != 0) { ... } need to check that the current name is terminated with '/' or '\0' -- not just a substring (dwd = "/doc_root", cwd = "/doc_root_of_all_evil"): dlen = strlen(dwd); if ((strncmp(cwd, dwd, dlen) != 0) || (cwd[dlen] && cwd[dlen] != '/')) { ... } Finally, the first thing main() should do is reset all the signal handlers and clear the environment: /* * Clear any signal handlers. */ (void) alarm(0); /* cancel any pending alarm */ for (i = 1; i < NSIG; ++i) { (void) signal(i, SIG_DFL); } /* * Start with a "clean" environment */ clean_env(); >How-To-Repeat: >Fix: --- suexec.c.orig Mon Jul 13 06:32:59 1998 +++ suexec.c Wed Aug 5 11:45:31 1998 @@ -111,46 +111,48 @@ extern char **environ; static FILE *log; +char *empty_ptr = (char *)0; /* for empty environment */ + char *safe_env_lst[] = { - "AUTH_TYPE", - "CONTENT_LENGTH", - "CONTENT_TYPE", - "DATE_GMT", - "DATE_LOCAL", - "DOCUMENT_NAME", - "DOCUMENT_PATH_INFO", - "DOCUMENT_ROOT", - "DOCUMENT_URI", - "FILEPATH_INFO", - "GATEWAY_INTERFACE", - "LAST_MODIFIED", - "PATH_INFO", - "PATH_TRANSLATED", - "QUERY_STRING", - "QUERY_STRING_UNESCAPED", - "REMOTE_ADDR", - "REMOTE_HOST", - "REMOTE_IDENT", - "REMOTE_PORT", - "REMOTE_USER", - "REDIRECT_QUERY_STRING", - "REDIRECT_STATUS", - "REDIRECT_URL", - "REQUEST_METHOD", - "REQUEST_URI", - "SCRIPT_FILENAME", - "SCRIPT_NAME", - "SCRIPT_URI", - "SCRIPT_URL", - "SERVER_ADMIN", - "SERVER_NAME", - "SERVER_PORT", - "SERVER_PROTOCOL", - "SERVER_SOFTWARE", - "UNIQUE_ID", - "USER_NAME", - "TZ", + "AUTH_TYPE=", + "CONTENT_LENGTH=", + "CONTENT_TYPE=", + "DATE_GMT=", + "DATE_LOCAL=", + "DOCUMENT_NAME=", + "DOCUMENT_PATH_INFO=", + "DOCUMENT_ROOT=", + "DOCUMENT_URI=", + "FILEPATH_INFO=", + "GATEWAY_INTERFACE=", + "LAST_MODIFIED=", + "PATH_INFO=", + "PATH_TRANSLATED=", + "QUERY_STRING=", + "QUERY_STRING_UNESCAPED=", + "REMOTE_ADDR=",+ "REMOTE_IDENT=", + "REMOTE_PORT=", + "REMOTE_USER=", + "REDIRECT_QUERY_STRING=", + "REDIRECT_STATUS=", + "REDIRECT_URL=", + "REQUEST_METHOD=", + "REQUEST_URI=", + "SCRIPT_FILENAME=", + "SCRIPT_NAME=", + "SCRIPT_URI=", + "SCRIPT_URL=", + "SERVER_ADMIN=", + "SERVER_NAME=", + "SERVER_PORT=", + "SERVER_PROTOCOL=", + "SERVER_SOFTWARE=", + "UNIQUE_ID=", + "USER_NAME=", + "TZ=", NULL }; @@ -199,10 +201,12 @@ { char pathbuf[512]; char **cleanenv; - char **ep; + char **ep, **envp; int cidx = 0; int idx; + envp = environ; + environ = &empty_ptr; /* VERY safe environment */ if ((cleanenv = (char **) calloc(AP_ENVBUF, sizeof(char *))) == NULL) { log_err("failed to malloc memory for environment\n"); @@ -209,10 +213,19 @@ } - for (ep = environ; *ep && cidx < AP_ENVBUF; ep++) { + sprintf(pathbuf, "PATH=%s", SAFE_PATH); + cleanenv[cidx] = strdup(pathbuf); + ++cidx; + for (ep = envp; *ep && cidx < AP_ENVBUF-1; ep++) { if (!strncmp(*ep, "HTTP_", 5)) { - cleanenv[cidx] = *ep; - cidx++; + char *cp = *ep; + while (*cp && (isalnum(*cp) || *cp == '_')) { + ++cp; + } + if (*cp == '=') { + cleanenv[cidx] = *ep; + cidx++; + } } else { for (idx = 0; safe_env_lst[idx]; idx++) { @@ -226,9 +239,7 @@ } } - sprintf(pathbuf, "PATH=%s", SAFE_PATH); - cleanenv[cidx] = strdup(pathbuf); - cleanenv[++cidx] = NULL; + cleanenv[cidx] = NULL; environ = cleanenv; } @@ -235,6 +246,8 @@ int main(int argc, char *argv[]) { + int dlen; /* length of document root directory name */ + int i; int userdir = 0; /* ~userdir flag */ uid_t uid; /* user information */ gid_t gid; /* target group placeholder */ @@ -253,6 +266,20 @@ struct stat prg_info; /* program info holder */ /* + * Clear any signal handlers. + */ + + (void) alarm(0); /* cancel any pending alarm */ + for (i = 1; i < NSIG; ++i) { + (void) signal(i, SIG_DFL); + } + + /* + * Start with a "clean" environment + */ + clean_env(); + + /* * If there are a proper number of arguments, set * all of them to variables. Otherwise, error out. */ @@ -324,6 +351,13 @@ } /* + * 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); + + /* * Error out if the target group name is invalid. */ if (strspn(target_gname, "1234567890") != strlen(target_gname)) { @@ -340,13 +374,6 @@ /* - * 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); - - /* * Log the transaction here to be sure we have an open log * before we setuid(). */ @@ -423,7 +450,8 @@ } } - if ((strncmp(cwd, dwd, strlen(dwd))) != 0) { + dlen = strlen(dwd); + if ((strncmp(cwd, dwd, dlen)) != 0 || (cwd[dlen] && cwd[dlen] != '/')) { log_err("command not in docroot (%s/%s)\n", cwd, cmd); exit(114); } @@ -492,8 +520,6 @@ log_err("file has no execute permission: (%s/%s)\n", cwd, cmd); exit(121); } - - clean_env(); /* * Be sure to close the log file so the CGI can't } exit(120); + "REMOTE_HOST=", >Audit-Trail: >Unformatted: [In order for any reply to be added to the PR database, ] [you need to include in the Cc line ] [and leave the subject line UNCHANGED. This is not done] [automatically because of the potential for mail loops. ] [If you do not include this Cc, your reply may be ig- ] [nored unless you are responding to an explicit request ] [from a developer. ] [Reply only with text; DO NOT SEND ATTACHMENTS! ]