On Sat, 2002-05-25 at 20:17, Colm MacCárthaigh wrote: > > Since there have been some changes to the affected source files > and multiple problems presented themselves in unixd.c, my patches > to make suexec + [ mod_include | mod_userdir | mod_cgid ] work > were getting stale. So I've rediffed them against CVS. > > I also had a good look through all of the suexec bugs, I'm using > the patches on a production system now with over 2000 shell users > (redbrick.dcu.ie) and it's proving stable. > > Anyway, I think they fix these : > > PR 7810 - suexec + mod_userdir + mod_cgid needed fixing (also > it's currently insecure by default, this really needs > to be fixed) > PR 7791 - malformed arguments array passed to suexec > PR 8291 - mod_include + suexec "exec cmd" not working > PR 9038 - really a duplicate of 7810 > > Some notes: > > 1: http://redbrick.dcu.ie/~colmmacc/patches/mod_cgid.patch > 2: http://redbrick.dcu.ie/~colmmacc/patches/unixd.patch > 3: http://redbrick.dcu.ie/~colmmacc/patches/mod_include.patch > > patch 1 (mod_cgid.c) fixes 7810/9039/mod_cgid, it just works.
Patch 1 didn't work when I tried it. The problem was that it depends on mod_userdir's get_suexec_id_doer() to create a proper suexec config based on the userdir. But mod_userdir's get_suexec_id_doer() function never is invoked, because mod_suexec's get_suexec_id_doer() is called first. I modified mod_userdir to insert its suexec identity function in front of mod_suexec's. The suexec+cgid+userdir combination now works properly in my test environment, but I'd like a second opinion before I commit. Thanks, --Brian
Index: modules/generators/mod_cgid.c =================================================================== RCS file: /home/cvs/httpd-2.0/modules/generators/mod_cgid.c,v retrieving revision 1.129 diff -u -r1.129 mod_cgid.c --- modules/generators/mod_cgid.c 17 May 2002 11:33:09 -0000 1.129 +++ modules/generators/mod_cgid.c 26 May 2002 20:01:07 -0000 @@ -329,16 +329,8 @@ if (rc != sizeof(int)) { return 1; } - rc = read(fd, &suexec_cfg->ugid.uid, sizeof(uid_t)); - if (rc != sizeof(uid_t)) { - return 1; - } - rc = read(fd, &suexec_cfg->ugid.gid, sizeof(gid_t)); - if (rc != sizeof(gid_t)) { - return 1; - } - rc = read(fd, &suexec_cfg->active, sizeof(int)); - if (rc != sizeof(int)) { + rc = read(fd, suexec_cfg, sizeof(*suexec_cfg)); + if (rc != sizeof(*suexec_cfg)) { return 1; } dconf[i] = (void *)suexec_cfg; @@ -379,12 +371,20 @@ } #endif #endif - /* For right now, just make the notes table. At some point we will need - * to actually fill this out, but for now we just don't want suexec to - * seg fault. - */ + + /* basic notes table to avoid segfaults */ r->notes = apr_table_make(r->pool, 1); + /* mod_userdir requires the mod_userdir_user note */ + rc = read(fd, &len, sizeof(len)); + if ((rc == sizeof(len)) && len) { + data = apr_pcalloc(r->pool, len + 1); /* last byte is '\0' */ + rc = read(fd, data, len); + if(rc != len) { + return 1; + } + apr_table_set(r->notes,"mod_userdir_user", data); + } return 0; } @@ -441,9 +441,9 @@ suexec_mod); write(fd, &suexec_mod->module_index, sizeof(int)); - write(fd, &suexec_cfg->ugid.uid, sizeof(uid_t)); - write(fd, &suexec_cfg->ugid.gid, sizeof(gid_t)); - write(fd, &suexec_cfg->active, sizeof(int)); + write(fd, suexec_cfg, sizeof(*suexec_cfg)); +ap_log_rerror(APLOG_MARK, APLOG_CRIT, 0, r, + "wrote suexec config %d bytes", sizeof(*suexec_cfg)); } #if 0 @@ -483,6 +483,16 @@ } #endif #endif + /* send a minimal notes table */ + data = (char *) apr_table_get(r->notes, "mod_userdir_user"); + if(data != NULL) { + len = strlen(data); + write(fd, &len, sizeof(len)); + write(fd, data, len); + } else { + len = 0; + write(fd, &len, sizeof(len)); + } } static void daemon_signal_handler(int sig) Index: modules/mappers/mod_userdir.c =================================================================== RCS file: /home/cvs/httpd-2.0/modules/mappers/mod_userdir.c,v retrieving revision 1.47 diff -u -r1.47 mod_userdir.c --- modules/mappers/mod_userdir.c 7 May 2002 00:17:11 -0000 1.47 +++ modules/mappers/mod_userdir.c 26 May 2002 20:01:07 -0000 @@ -366,7 +366,7 @@ } #ifdef HAVE_UNIX_SUEXEC -static ap_unix_identity_t *get_suexec_id_doer(const request_rec *r) +static ap_unix_identity_t *userdir_get_suexec_id_doer(const request_rec *r) { ap_unix_identity_t *ugid = NULL; #if APR_HAS_USER @@ -396,7 +396,7 @@ ap_hook_translate_name(translate_userdir,aszPre,NULL,APR_HOOK_MIDDLE); #ifdef HAVE_UNIX_SUEXEC - ap_hook_get_suexec_identity(get_suexec_id_doer,NULL,NULL,APR_HOOK_MIDDLE); + ap_hook_get_suexec_identity(userdir_get_suexec_id_doer,NULL,NULL,APR_HOOK_FIRST); #endif }