Re: [mod_fcgid PATCH] catch errors from setuid()/seteuid()
On Wed, Jan 20, 2010 at 8:19 PM, pqf p...@mailtech.cn wrote: I man seteuid in my Linux box, there are two types of errors: ERRORS The seteuid() function shall fail if: EINVAL The value of the uid argument is invalid and is not supported by the implementation. EPERM The process does not have appropriate privileges and uid does not match the real group ID or the saved set-group- ID. If directly pass 0 in setuid(), EINVAL may not happend If this process is seteuid from root, EPERM may not happend so, I think the check is just a textbook logic check? yes, until somebody changes code or some other bug results in this being called in a different environment just call _exit(1) if it fail? two concerns with that minimal change: 1. seteuid() works once then fails n times now (at least on Solaris), so some extra logic is needed 2. even if these calls never fail, the presence of the exit() without a log message may cause somebody to lose a lot of time investigating a mysterious disappearance of the new process --/-- I'll punt on this until after 2.3.5 since I'd like to spend the time to watch it work on another platform or two. (suexec is not something I use more than once every ~3 years, so it is worth setting up in multiple environments.)
Re: [mod_fcgid PATCH] catch errors from setuid()/seteuid()
Hi, Jeff Your concerns are right, +1 for your patch :) Thanks -- From: Jeff Trawick traw...@gmail.com Sent: Thursday, January 21, 2010 9:23 PM To: dev@httpd.apache.org Subject: Re: [mod_fcgid PATCH] catch errors from setuid()/seteuid() On Wed, Jan 20, 2010 at 8:19 PM, pqf p...@mailtech.cn wrote: I man seteuid in my Linux box, there are two types of errors: ERRORS The seteuid() function shall fail if: EINVAL The value of the uid argument is invalid and is not supported by the implementation. EPERM The process does not have appropriate privileges and uid does not match the real group ID or the saved set-group- ID. If directly pass 0 in setuid(), EINVAL may not happend If this process is seteuid from root, EPERM may not happend so, I think the check is just a textbook logic check? yes, until somebody changes code or some other bug results in this being called in a different environment just call _exit(1) if it fail? two concerns with that minimal change: 1. seteuid() works once then fails n times now (at least on Solaris), so some extra logic is needed 2. even if these calls never fail, the presence of the exit() without a log message may cause somebody to lose a lot of time investigating a mysterious disappearance of the new process --/-- I'll punt on this until after 2.3.5 since I'd like to spend the time to watch it work on another platform or two. (suexec is not something I use more than once every ~3 years, so it is worth setting up in multiple environments.)
[mod_fcgid PATCH] catch errors from setuid()/seteuid()
During the last hackathon, Paul was kind enough to run the clang/llvm
static analysis on mod_fcgid
(http://zeus.kimaker.com/~chip/fcgid-scan/). That pointed out these
setuid()/seteuid() calls that aren't checked prior to running a child.
The error checking itself is simple enough, but there's an ugly aspect
of the implementation that results in trying to switch effective/real
uids multiple times that I worked around. (See the FIXME text in the
patch. I'm not aware of a simple solution, especially one simple
enough to get into 2.3.5) The seteuid() call would otherwise fail on
subsequent invocations for the same child.
IIRC Joe thought that the seteuid() wasn't needed at all, but the
setuid() fails without it on Solaris.
Concerns?
Is there some reason that testing on Linux and Solaris wouldn't be sufficient?
Index: modules/fcgid/fcgid_proc_unix.c
===
--- modules/fcgid/fcgid_proc_unix.c (revision 901320)
+++ modules/fcgid/fcgid_proc_unix.c (working copy)
@@ -160,10 +160,44 @@
return APR_SUCCESS;
}
+static void log_setid_failure(const char *id_type,
+ uid_t user_id)
+{
+char errno_desc[120];
+char errmsg[240];
+
+apr_strerror(errno, errno_desc, sizeof errno_desc);
+apr_snprintf(errmsg, sizeof errmsg,
+ (%d)%s: mod_fcgid child unable to set %s to %ld\n,
+ errno, errno_desc, id_type, (long)user_id);
+write(STDERR_FILENO, errmsg, strlen(errmsg));
+}
+
+/* FIXME
+ * When using suexec, mod_fcgid registers this child cleanup
+ * with the pool for every FastCGI application as a way of
+ * switching user ids in the child before running suexec.
+ * But such a cleanup will be run for every such pool. IOW,
+ * when creating the third FastCGI application process this
+ * cleanup will run three times before exec-ing.
+ * ap_unixd_config.user_id is invariant, so this does not
+ * cause an operational problem. Note that seteuid(0) only
+ * works the first time, or until the first setuid(), so skip
+ * subsequent calls by checking the uid.
+ */
static apr_status_t exec_setuid_cleanup(void *dummy)
{
-seteuid(0);
-setuid(ap_unixd_config.user_id);
+if (getuid() != ap_unixd_config.user_id) {
+if (seteuid(0) == -1) {
+log_setid_failure(effective uid, 0);
+_exit(1);
+}
+
+if (setuid(ap_unixd_config.user_id) == -1) {
+log_setid_failure(uid, ap_unixd_config.user_id);
+_exit(1);
+}
+}
return APR_SUCCESS;
}
@@ -227,7 +261,10 @@
return errno;
}
-/* Unlink it when process exit */
+/* Register a cleanup to
+ * 1. Unlink the socket when the process exits
+ * 2. (suexec mode only, in the child cleanup) Switch to the configured uid
+ */
if (ap_unixd_config.suexec_enabled) {
apr_pool_cleanup_register(procnode-proc_pool,
procnode, socket_file_cleanup,
Re: [mod_fcgid PATCH] catch errors from setuid()/seteuid()
I man seteuid in my Linux box, there are two types of errors: ERRORS The seteuid() function shall fail if: EINVAL The value of the uid argument is invalid and is not supported by the implementation. EPERM The process does not have appropriate privileges and uid does not match the real group ID or the saved set-group- ID. If directly pass 0 in setuid(), EINVAL may not happend If this process is seteuid from root, EPERM may not happend so, I think the check is just a textbook logic check? just call _exit(1) if it fail? -- From: Jeff Trawick traw...@gmail.com Sent: Thursday, January 21, 2010 5:38 AM To: dev@httpd.apache.org Subject: [mod_fcgid PATCH] catch errors from setuid()/seteuid() During the last hackathon, Paul was kind enough to run the clang/llvm static analysis on mod_fcgid (http://zeus.kimaker.com/~chip/fcgid-scan/). That pointed out these setuid()/seteuid() calls that aren't checked prior to running a child. The error checking itself is simple enough, but there's an ugly aspect of the implementation that results in trying to switch effective/real uids multiple times that I worked around. (See the FIXME text in the patch. I'm not aware of a simple solution, especially one simple enough to get into 2.3.5) The seteuid() call would otherwise fail on subsequent invocations for the same child. IIRC Joe thought that the seteuid() wasn't needed at all, but the setuid() fails without it on Solaris. Concerns? Is there some reason that testing on Linux and Solaris wouldn't be sufficient?
