You are viewing a plain text version of this content. The canonical link for it is here.
Posted to apache-bugdb@apache.org by Jeff Stewart <jw...@purdue.edu> on 1998/08/05 20:58:38 UTC
suexec/2790: Array overflow in suexec and some suggested improvements
>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 <ap...@Apache.Org> 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! ]