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
Please help us to refine our list of open and current defects; this is a mass update of old and inactive Bugzilla reports which reflect user error, already resolved defects, and still-existing defects in httpd. As repeatedly announced, the Apache HTTP Server Project has discontinued all development and patch review of the 2.2.x series of releases. The final release 2.2.34 was published in July 2017, and no further evaluation of bug reports or security risks will be considered or published for 2.2.x releases. All reports older than 2.4.x have been updated to status RESOLVED/LATER; no further action is expected unless the report still applies to a current version of httpd. If your report represented a question or confusion about how to use an httpd feature, an unexpected server behavior, problems building or installing httpd, or working with an external component (a third party module, browser etc.) we ask you to start by bringing your question to the User Support and Discussion mailing list, see [https://httpd.apache.org/lists.html#http-users] for details. Include a link to this Bugzilla report for completeness with your question. If your report was clearly a defect in httpd or a feature request, we ask that you retest using a modern httpd release (2.4.33 or later) released in the past year. If it can be reproduced, please reopen this bug and change the Version field above to the httpd version you have reconfirmed with. Your help in identifying defects or enhancements still applicable to the current httpd server software release is greatly appreciated.
Yep, this bug report is still relevant. In the last 13 years, only the null check for strdup has been added. The other 6 issues mentioned in this bug report are still open. The appended patch probably won't apply cleanly after waiting 13 years, but the idea should still be very clear.
Where is the dependency on sizeof(int) exactly, and what's the logging problem after execve failure? I am not sure that setting environ[x] to an unwritable string literal is actually safe (environ is declared as char **), I changed it to a simpler strdup in r1846253. I don't see a variable "prog" in the current code.
Sorry, my bad, I didn't look close enough. The issue with atoi is still there. That function should never be used in any code. Not even when you know that the string only consists of digits, since there is still the possibility of overflow. Undefined behavior. ;) The sprintf call may still overflow the buffer. The "unable to log" refers to the "exec failed" at the very bottom of the file. At that point, the log files have been closed (see closelog and fclose further above), and since the process has changed ownership by then, it will not be able to write to the suexec log file. Luckily this is only in very specific circumstances (file not executable even though it has the executable bit set), so it would probably not happen too often. Still the code should be solid here. All other items from the original report have been fixed in the meantime.
sprintf was removed entirely in the commit above. By inspection err_output() should re-open the log file after it was closed, though I haven't tested that.
(In reply to Joe Orton from comment #5) > sprintf was removed entirely in the commit above. Thank you for fixing this. I my comment #4 I must have overlooked that the sprintf is gone. I'm not sure why I overlooked it since https://svn.apache.org/viewvc/ shows all changes immediately, so I should have seen your fix already when looking at the HEAD revision. Maybe my browser showed an outdated version. Thanks again.
I you want to test whether the logging fails, it should be possible have an ELF binary for another architecture in cgi-bin. This scenario should trigger exactly that code path. Background and intentions The reason I'm so picky with the suexec code is that for me, back in 2005, it was the primary example of how to program very carefully. It's a setuid binary after all, therefore it should demonstrate best practices and document many pitfalls. And it does a great job at this, serving well for education. Therefore, and because it is such a prominent example of security critical code, I wanted it to be perfect.
Very good intentions! Good idea about making execve fail, made me think of a bogus shebang line which fails execve as well. I tested it with suexec as built in Fedora (which has syslog) and it logged it fine, FYI - # journalctl --no-hostname -r -u httpd | head -3 -- Logs begin at Wed 2017-11-08 10:55:43 GMT, end at Tue 2018-11-13 16:22:05 GMT. -- Nov 13 16:19:22 suexec[707]: (2)No such file or directory: exec failed (env.sh) Nov 13 16:19:22 suexec[707]: uid: (1000/jorton) gid: (1000/jorton) cmd: env.sh I will fix the atoi as well.
Re-opened, because of the 'atoi' still around (see comment #8)
Remove atoi() use in 1876744 and tried to be more paranoid about integer conversion there too.