[mod_fcgid PATCH] catch errors from setuid()/seteuid()

[Jeff Trawick]

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,