httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aaron Bannert <aa...@clove.org>
Subject [PATCH] suexec to work with relative paths
Date Wed, 14 Nov 2001 23:38:53 GMT
I'd like some peer review of this patch before it gets committed. I'm mostly
concerned about hidden security concerns in the suexec/mod_suexec code.

Fixes:

 - Apache install directory can be relocated now that suexec deals with
   relative paths for: suexec's docroot, suexec's binary path, and suexec's
   log path.

Features:

 - Adds configure-time option to set suexec binary path -- if not absolute
   it is assumed to be relative to the ServerRoot.

Implementation Details:

 - Apache forms absolute paths for each of the above suexec pathnames,
   even if they are presented in a relative form.

 - The logpath and suexec docroot (not the main docroot) are passed as
   new parameters to the suexec call. (This seems OK to me, since we're
   already "trusting" the other argv params passed to suexec.)

Issues:

 - If suexec's first call to getpwuid() fails, it's probably not going to get
   logged anywhere, but that doesn't matter since if it was a relative
   path it wasn't getting logged anyway.

 - I need more people to examine these changes for possible security holes.

Verified in these scenarios:

 - prefork and worker
 - original install directory and relocated directory
 - absolute and relative paths for each suexec path parameter
 - linux 2.4/intel and solaris8/intel
 - with DSOs and static modules

-aaron



Index: configure.in
===================================================================
RCS file: /home/cvs/httpd-2.0/configure.in,v
retrieving revision 1.185
diff -u -r1.185 configure.in
--- configure.in	2001/10/16 05:18:39	1.185
+++ configure.in	2001/11/09 17:35:53
@@ -304,6 +304,12 @@
   progname="httpd"] )
 
 # SuExec parameters
+AC_ARG_WITH(suexec-bin,
+APACHE_HELP_STRING(--with-suexec-bin,Path to suexec binary),[
+  suexec_bin="$withval" ], [
+  suexec_bin="$prefix/bin/suexec" ] )
+AC_DEFINE_UNQUOTED(SUEXEC_BIN, "$suexec_bin", [Path to suexec binary] )
+
 AC_ARG_WITH(suexec-caller,
 APACHE_HELP_STRING(--with-suexec-caller,User allowed to call SuExec),[
   AC_DEFINE_UNQUOTED(AP_HTTPD_USER, "$withval", [User allowed to call SuExec] ) ] )
Index: modules/generators/mod_cgi.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/generators/mod_cgi.c,v
retrieving revision 1.106
diff -u -r1.106 mod_cgi.c
--- modules/generators/mod_cgi.c	2001/09/18 03:33:30	1.106
+++ modules/generators/mod_cgi.c	2001/11/09 17:35:54
@@ -374,6 +374,7 @@
                                   exec_info *e_info)
 {
     const char * const *env;
+    const char *doc_root;
     apr_procattr_t *procattr;
     apr_proc_t *procnew;
     apr_status_t rc = APR_SUCCESS;
@@ -417,6 +418,10 @@
 	fprintf(dbg, "'%s'\n", env[i]);
 #endif
 
+    doc_root = ap_server_root_relative(r->pool, 
+                                       ap_make_dirstr_parent(r->pool,
+                                                             r->filename));
+
     /* Transmute ourselves into the script.
      * NB only ISINDEX scripts get decoded arguments.
      */
@@ -425,8 +430,7 @@
                                   e_info->in_pipe,
                                   e_info->out_pipe,
                                   e_info->err_pipe)) != APR_SUCCESS) ||
-        ((rc = apr_procattr_dir_set(procattr, 
-                                  ap_make_dirstr_parent(r->pool, r->filename))) !=
APR_SUCCESS) ||
+        ((rc = apr_procattr_dir_set(procattr, doc_root) != APR_SUCCESS)) ||
 #ifdef RLIMIT_CPU
         ((rc = apr_procattr_limit_set(procattr, APR_LIMIT_CPU, conf->limit_cpu)) != APR_SUCCESS)
||
 #endif
Index: modules/generators/mod_cgid.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/generators/mod_cgid.c,v
retrieving revision 1.99
diff -u -r1.99 mod_cgid.c
--- modules/generators/mod_cgid.c	2001/10/12 21:39:40	1.99
+++ modules/generators/mod_cgid.c	2001/11/09 17:35:55
@@ -505,6 +505,7 @@
         char *argv0; 
         char **env; 
         const char * const *argv; 
+        const char *doc_root;
         apr_int32_t   in_pipe  = APR_CHILD_BLOCK;
         apr_int32_t   out_pipe = APR_CHILD_BLOCK;
         apr_int32_t   err_pipe = APR_CHILD_BLOCK;
@@ -541,6 +542,10 @@
             cmd_type = APR_SHELLCMD;
         }
 
+        doc_root = ap_server_root_relative(r->pool,
+                                           ap_make_dirstr_parent(r->pool,
+                                                                 r->filename));
+
         if (((rc = apr_procattr_create(&procattr, ptrans)) != APR_SUCCESS) ||
             ((req_type == CGI_REQ) && 
              (((rc = apr_procattr_io_set(procattr,
@@ -554,8 +559,7 @@
               ((rc = apr_procattr_child_err_set(procattr, r->server->error_log, NULL))
!= APR_SUCCESS) ||
               ((rc = apr_procattr_child_in_set(procattr, inout, NULL)) != APR_SUCCESS)))
||
             ((rc = apr_procattr_child_out_set(procattr, inout, NULL)) != APR_SUCCESS) ||
-            ((rc = apr_procattr_dir_set(procattr,
-                                  ap_make_dirstr_parent(r->pool, r->filename))) !=
APR_SUCCESS) ||
+            ((rc = apr_procattr_dir_set(procattr, doc_root)) != APR_SUCCESS) ||
             ((rc = apr_procattr_cmdtype_set(procattr, cmd_type)) != APR_SUCCESS)) {
             /* Something bad happened, tell the world. */
             ap_log_rerror(APLOG_MARK, APLOG_ERR, rc, r,
Index: os/unix/unixd.c
===================================================================
RCS file: /home/cvs/httpd-2.0/os/unix/unixd.c,v
retrieving revision 1.41
diff -u -r1.41 unixd.c
--- os/unix/unixd.c	2001/10/19 23:32:43	1.41
+++ os/unix/unixd.c	2001/11/09 17:35:55
@@ -234,10 +234,13 @@
     unixd_config.user_name = DEFAULT_USER;
     unixd_config.user_id = ap_uname2id(DEFAULT_USER);
     unixd_config.group_id = ap_gname2id(DEFAULT_GROUP);
+    unixd_config.suexec_bin = ap_server_root_relative(ptemp, SUEXEC_BIN);
+    unixd_config.suexec_docroot = ap_server_root_relative(ptemp, AP_DOC_ROOT);
+    unixd_config.suexec_logfile = ap_server_root_relative(ptemp, AP_LOG_EXEC);
 
     /* Check for suexec */
     unixd_config.suexec_enabled = 0;
-    if ((apr_stat(&wrapper, SUEXEC_BIN, 
+    if ((apr_stat(&wrapper, unixd_config.suexec_bin, 
                   APR_FINFO_NORM, ptemp)) != APR_SUCCESS) {
         return;
     }
@@ -345,16 +348,18 @@
             i++;
 	    }
     }
-    newargs = apr_palloc(p, sizeof(char *) * (i + 4));
-    newprogname = SUEXEC_BIN;
-    newargs[0] = SUEXEC_BIN;
-    newargs[1] = execuser;
-    newargs[2] = execgroup;
-    newargs[3] = apr_pstrdup(p, progname);
+    newargs = apr_palloc(p, sizeof(char *) * (i + 6));
+    newprogname = apr_pstrdup(p, unixd_config.suexec_bin);
+    newargs[0] = newprogname;
+    newargs[1] = apr_pstrdup(p, unixd_config.suexec_docroot);
+    newargs[2] = apr_pstrdup(p, unixd_config.suexec_logfile);
+    newargs[3] = execuser;
+    newargs[4] = execgroup;
+    newargs[5] = apr_pstrdup(p, progname);
 
     i = 0;
     do {
-        newargs[i + 4] = args[i];
+        newargs[i + 6] = args[i];
     } while (args[i++]);
 
     return apr_proc_create(newproc, newprogname, newargs, env, attr, p);
Index: os/unix/unixd.h
===================================================================
RCS file: /home/cvs/httpd-2.0/os/unix/unixd.h,v
retrieving revision 1.29
diff -u -r1.29 unixd.h
--- os/unix/unixd.h	2001/10/19 23:32:43	1.29
+++ os/unix/unixd.h	2001/11/09 17:35:56
@@ -105,6 +105,9 @@
     uid_t user_id;
     gid_t group_id;
     int suexec_enabled;
+    const char *suexec_bin;
+    const char *suexec_docroot;
+    const char *suexec_logfile;
 } unixd_config_rec;
 AP_DECLARE_DATA extern unixd_config_rec unixd_config;
 
Index: support/suexec.c
===================================================================
RCS file: /home/cvs/httpd-2.0/support/suexec.c,v
retrieving revision 1.16
diff -u -r1.16 suexec.c
--- support/suexec.c	2001/10/30 17:38:03	1.16
+++ support/suexec.c	2001/11/09 17:35:56
@@ -121,6 +121,11 @@
 #define AP_ENVBUF 256
 
 extern char **environ;
+#ifdef AP_LOG_EXEC
+static char *logfile = AP_LOG_EXEC;
+#else
+static char *logfile = "";
+#endif /* AP_LOG_EXEC */
 static FILE *log = NULL;
 
 char *safe_env_lst[] =
@@ -175,7 +180,7 @@
     struct tm *lt;
 
     if (!log) {
-	if ((log = fopen(AP_LOG_EXEC, "a")) == NULL) {
+	if ((log = fopen(logfile, "a")) == NULL) {
 	    fprintf(stderr, "failed to open log file\n");
 	    perror("fopen");
 	    exit(1);
@@ -253,6 +258,7 @@
     int userdir = 0;		/* ~userdir flag             */
     uid_t uid;			/* user information          */
     gid_t gid;			/* target group placeholder  */
+    char *docroot;              /* merged absolute docroot path */
     char *target_uname;		/* target user name          */
     char *target_gname;		/* target group name         */
     char *target_homedir;	/* target home directory     */
@@ -301,7 +307,7 @@
         fprintf(stderr, " -D AP_HTTPD_USER=\"%s\"\n", AP_HTTPD_USER);
 #endif
 #ifdef AP_LOG_EXEC
-        fprintf(stderr, " -D AP_LOG_EXEC=\"%s\"\n", AP_LOG_EXEC);
+        fprintf(stderr, " -D AP_LOG_EXEC=\"%s\"\n", logfile);
 #endif
 #ifdef AP_SAFE_PATH
         fprintf(stderr, " -D AP_SAFE_PATH=\"%s\"\n", AP_SAFE_PATH);
@@ -325,9 +331,11 @@
 	log_err("too few arguments\n");
 	exit(101);
     }
-    target_uname = argv[1];
-    target_gname = argv[2];
-    cmd = argv[3];
+    docroot = argv[1];
+    logfile = argv[2];
+    target_uname = argv[3];
+    target_gname = argv[4];
+    cmd = argv[5];
 
     /*
      * Check to see if the user running this program
@@ -504,10 +512,10 @@
 	}
     }
     else {
-	if (((chdir(AP_DOC_ROOT)) != 0) ||
+	if (((chdir(docroot)) != 0) ||
 	    ((getcwd(dwd, AP_MAXPATH)) == NULL) ||
 	    ((chdir(cwd)) != 0)) {
-	    log_err("cannot get docroot information (%s)\n", AP_DOC_ROOT);
+	    log_err("cannot get docroot information (%s)\n", docroot);
 	    exit(113);
 	}
     }
@@ -599,7 +607,7 @@
      * mess with it.  If the exec fails, it will be reopened 
      * automatically when log_err is called.  Note that the log
      * might not actually be open if AP_LOG_EXEC isn't defined.
-     * However, the "log" cell isn't ifdef'd so let's be defensive
+     * However, the "log" call isn't ifdef'd so let's be defensive
      * and assume someone might have done something with it
      * outside an ifdef'd AP_LOG_EXEC block.
      */
@@ -616,10 +624,10 @@
     {
 	extern char **environ;
 
-	ap_execve(cmd, &argv[3], environ);
+	ap_execve(cmd, &argv[5], environ);
     }
 #else /*NEED_HASHBANG_EMUL*/
-    execv(cmd, &argv[3]);
+    execv(cmd, &argv[5]);
 #endif /*NEED_HASHBANG_EMUL*/
 
     /*

Mime
View raw message