[PATCH 2.0] PR 31302 suexec doesn't execute commands if they're not in the current dir

[Kaspar Brand <de...@velox.ch>]

At 08:47 PM 6/2/2005, Bill wrote:
> One more useful clue, keyword it PatchAvailable once you have
> a solution (attached) so that committers seeking low-hanging fruit
> can prune some off and close the bugs.

... this is what I was trying to do for PR 31302 (hoping that the fix would 
find its way into 2.0.54), but obviously my posting to the mailing list on 3 
April didn't catch anybody's attention... so I'm making a second attempt. 
Cf. also

   http://issues.apache.org/bugzilla/show_bug.cgi?id=31302

For the sake of completeness, I'm attaching the patch to this message (in 
the meantime, I realized that apr_pstrdup is not necessary at that point, 
actually the whole line can just be replaced by "cmdpath = parsed_string").

Comments welcome, of course - and looking forward to 2.0.55...

Kaspar 

Re: [PATCH 2.0] PR 31302 suexec doesn't execute commands if they're not in the current dir

[Kaspar Brand <de...@velox.ch>]

> At 11:23 PM 6/4/2005, William A. Rowe, Jr. wrote:
>
>>I'm actually concerned at a higher level that r->filename CANNOT
>>be relative.  Was this the case coming out of mod_include??
>>
>>r->filename is run up against <Directory > sections, so apparently
>>if r->filename is incomplete, mod_include was wrong.
>
> Ugh - unless this is mod_include exec_cgi (v.s. vitural)

Thanks for jumping in, Bill! Let me try to illustrate the issue(s) with an 
example (sorry if this is all obvious, but maybe it helps to make things 
more clear).

Note that my proposed patch is only concerned with "#exec cmd", that's what 
has been moved to mod_cgi (prior to 2.0.10 already). "#include virtual" is 
not affected by my patch, this is still handled in mod_include. Let's assume 
we have a file test.shtml in a user's public_html directory with these SSI 
tags:

   <!--#exec cmd="subdir/test.sh"-->
   <!--#exec cmd="/home/someuser/public_html/subdir/test.sh"-->

r->filename will always be set to "/home/someuser/public_html/test.shtml" 
(i.e., an absolute path), that's not the issue. The current mod_cgi code, 
however, sets the current directory for executing the "cmd" command to the 
directory holding r->filename (mod_cgi.c, line 422):

   ((rc = apr_procattr_dir_set(procattr,
                   ap_make_dirstr_parent(r->pool,
                                         r->filename))) != APR_SUCCESS) ||

and finally, ap_unix_create_privileged_process in os/unix/unixd.c removes 
everything in the "cmd" parameter up to the last slash:

   argv0 = ap_strrchr_c(progname, '/');
   /* Allow suexec's "/" check to succeed */
   if (argv0 != NULL) {
       argv0++;
   }
   else {
       argv0 = progname;
   }

which means that suexec won't be able to execute the file unless it's in the 
same directory (i.e., the current dir will be set to 
"/home/someuser/public_html", and suexec will try to find "test.sh" in this 
directory, not in "subdir").

What my patch does is actually:

1) Making sure that when parsing SSI #exec tags (handle_exec() in 
mod_cgi.c), relative paths given in the "cmd" parameter are converted to 
absolute paths (by taking r->filename and using apr_filepath_merge());

2) Setting the current directory for the CGI child (run_cgi_child() in 
mod_cgi.c) to the current directory of the command to be executed (not 
r->filename). Normally, r->filename and the "command" argument supplied to 
run_cgi_child() are the same, cf. default_build_command() in mod_cgi.c (line 
500):

   if (e_info->process_cgi) {
       *cmd = r->filename;

... but in the case of SSI #exec, r->filename and command differ (and if 
they're not in the same directory, ap_make_dirstr_parent() will have 
different results, of course).

> in which case there's a host of ramifications.  My sixth sense
> tells me this isn't an issue, since suexec will only run code
> within the suexec document tree, but again, it's not a trivial
> patch, and it's repercussions are far from trivial.

Agreed. The second point above is actually the non-trivial one, but I hope 
to have shown that its ramifications are limited.

BTW, even with my patch there will be a small difference in the behavior 
under Apache 1.3 and 2.0 (restoring the old behavior would mean fiddling 
with os/unix/unixd.c): when using

   <#exec cmd="subdir/test.sh">

in a file /home/auser/public_html/test.shtml, Apache 1.3 will set the 
current dir to /home/auser/public_html (and call suexec with 
"subdir/test.sh"), while Apache 2.0 will set it to 
/home/auser/public_html/subdir (and call suexec with "test.sh"). This will 
have ramifications if test.sh uses relative paths, but in my opinion, the 
behavior under Apache 1.3 is somewhat odd, I would prefer the "new" Apache 2 
way.

Hopefully this sheds some more light on the issue.

Thanks,
Kaspar 

Re: [PATCH 2.0] PR 31302 suexec doesn't execute commands if they're not in the current dir

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

At 12:25 PM 6/4/2005, William A. Rowe, Jr. wrote:

>I'm actually concerned at a higher level that r->filename CANNOT
>be relative.  Was this the case coming out of mod_include??
>
>r->filename is run up against <Directory > sections, so apparently
>if r->filename is incomplete, mod_include was wrong.

Ugh - unless this is mod_include exec_cgi (v.s. vitural)

in which case there's a host of ramifications.  My sixth sense
tells me this isn't an issue, since suexec will only run code
within the suexec document tree, but again, it's not a trivial
patch, and it's repercussions are far from trivial.

Bill

Re: [PATCH 2.0] PR 31302 suexec doesn't execute commands if they're not in the current dir

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

At 10:00 AM 6/4/2005, Kaspar Brand wrote:
>At 08:47 PM 6/2/2005, Bill wrote:
>>One more useful clue, keyword it PatchAvailable once you have
>>a solution (attached) so that committers seeking low-hanging fruit
>>can prune some off and close the bugs.
>
>... this is what I was trying to do for PR 31302 (hoping that the fix would find its way into 2.0.54), 

Thanks - you have to admit any suggested patch to suexec (which is
what the bug summary still says) will scare away anyone looking for
low-hanging fruit.  That said, you did come up with a potentially
better solution.  But it will take someone with a solid 1/2 day to
delve into all the side effects.

>but obviously my posting to the mailing list on 3 April didn't catch anybody's attention... so I'm making a second attempt. Cf. also
>
>  http://issues.apache.org/bugzilla/show_bug.cgi?id=31302

Good idea, thanks for the [PATCH] subject:

>For the sake of completeness, I'm attaching the patch to this message (in the meantime, I realized that apr_pstrdup is not necessary at that point, actually the whole line can just be replaced by "cmdpath = parsed_string").
>
>Comments welcome, of course - and looking forward to 2.0.55...

Saving folks a long read through the suexec info; your comments:

<quote>


> mod_include needs a patch that turns CGI script paths into absolute paths.

That's what my proposed fix tries to do. The code handling the SSI exec tag 
has actually been moved to mod_cgi (before 2.0.10 already), so that's where 
the patch goes.

In handle_exec(), I have added a check to make sure an absolute pathname is 
used when calling include_cmd()/run_cgi_child(). For suexec, the important 
point is that in run_cgi_child() the directory for the new child is set to the 
parent of "command", not the one of r->filename (they differ if the "exec" 
target is not in the same directory as the shtml file).

I've tested this both with relative and absolute file names for the "cmd" 
parameter, and running with and without suexec. It works for me (as predicted 
by Ryan), but further testing is welcome, of course. And finally, I'd 
appreciate to see this fix (or a similar one) in 2.0.54... thanks!


</quote>


I'm actually concerned at a higher level that r->filename CANNOT
be relative.  Was this the case coming out of mod_include??

r->filename is run up against <Directory > sections, so apparently
if r->filename is incomplete, mod_include was wrong.

Bill