On Wed, 02 Mar 2005 08:10:03 -0500, Stas Bekman <[EMAIL PROTECTED]> wrote:
Damon Buckwalter wrote:
On Tue, 01 Mar 2005 21:55:00 -0500, Stas Bekman <[EMAIL PROTECTED]> wrote:
Just because everyone is jumping off of bridges, does not mean we should... ;^) The calls are insecure because the file permissions can change between the stat() performed by filetests and the call to apr_file_open(),
Unrelated to the discussed problem, in our particular case, I can't see what's insecure about that. I believe that check was added to give a better diagnostics at an earlier stage. If someone manages to kill the -x bits after it was checked, I can't see what security damage could be done.
Well, I won't press this too hard, since I believe the ideal solution removes the -x and -r checks anyway. Probably it is not insecure, just a little unreliable.
failing to open a file will cover the -r check. What will cover the -x check? Or do you suggest that we shouldn't require cgi scripts to be executable? At least mod_cgi requires so when doing exec, but we don't exec. So we should probably keep it in, no?
[...]
I tried this, dropping the -x and -r checks _alone_ does not work as I would think is intended. With just those checks removed, and if this is my config:
<Files *.cgi> SetHandler perl-script PerlResponseHandler ModPerl::PerlRun PerlOptions +ParseHeaders Options +ExecCGI </Files>
Scripts accessible only via ACL permissions work okay, but a 500 Internal server error is returned for *.cgi files that do not exist in the filesystem. This is because the SLURP_SUCCESS method croaks on any error, as you show below.
e.g. a request for /mp/bar.cgi, which does not exist:
[Mon Feb 28 23:10:02 2005] [error] [client ::1] Error opening '/home/damon/test/ apache2/htdocs/mp/bar.cgi': No such file or directory at /home/damon/test/perl/ lib/site_perl/5.9.1/i686-linux-thread-multi-64int-ld/ModPerl/RegistryCooker.pm l ine 546.\n
The browser gets a 500, but I believe that a 404 would be more helpful. That means changing slurp_filename/SLURP_SUCCESS to return the apropriate Apache status code (404, 403, etc) instead of just returning the content of the file to be slurped or croaking if it cannot be loaded.
This is where I start to get lost in the mod_perl code, so I'm hoping someone understands the problem and how to solve it better than I do in the code.
Please take a look at the attached patch. I believe it does what you want. The only remaining concern of mine is the -x bit.
I hope that I have made myself more clear this time around, and that the example from Apache httpd helps you believe that I am not just halucinating. ;^)
I've never suggested that you were halucinating, Damon. I'm just trying to figure out how to make you happy, w/o making someone else unhappy :)
Fair enough. I'm trying to do the same, but struggling a bit with the guts of mod_perl, etc. to get there. I'm trying my best not to just be a 'seagull': showing up, making noise, sh*ting on things and leaving ;^)
No worries, Damon, you are doing a good job.
Here is the patch (3 sub tests now fail since we no longer report FORBIDDEN when -x is not there):
Index: ModPerl-Registry/lib/ModPerl/RegistryCooker.pm
===================================================================
--- ModPerl-Registry/lib/ModPerl/RegistryCooker.pm (revision 155373)
+++ ModPerl-Registry/lib/ModPerl/RegistryCooker.pm (working copy)
@@ -254,21 +254,10 @@
my $self = shift;
my $r = $self->{REQ};- unless (-r $r->my_finfo && -s _) {
- $self->log_error("$self->{FILENAME} not found or unable to stat");
- return Apache::NOT_FOUND;
- }
+ return Apache::DECLINED if -d $r->my_finfo;- return Apache::DECLINED if -d _;
-
$self->{MTIME} = -M _;- unless (-x _ or IS_WIN32) {
- $r->log_error("file permissions deny server execution",
- $self->{FILENAME});
- return Apache::FORBIDDEN;
- }
-
if (!($r->allow_options & Apache::OPT_EXECCGI)) {
$r->log_error("Options ExecCGI is off in this directory",
$self->{FILENAME});
@@ -372,10 +361,13 @@
sub convert_script_to_compiled_handler {
my $self = shift;+ my $rc = Apache::OK;
+
$self->debug("Adding package $self->{PACKAGE}") if DEBUG & D_NOISE;# get the script's source - $self->read_script; + $rc = $self->read_script; + return $rc unless $rc == Apache::OK;
# convert the shebang line opts into perl code
$self->rewrite_shebang;
@@ -408,7 +400,7 @@
${ $self->{CODE} },
"\n}"; # last line comment without newline?- my $rc = $self->compile(\$eval);
+ $rc = $self->compile(\$eval);
return $rc unless $rc == Apache::OK;
$self->debug(qq{compiled package \"$self->{PACKAGE}\"}) if DEBUG & D_NOISE;
@@ -534,16 +526,23 @@ # dflt: read_script # desc: reads the script in # args: $self - registry blessed object -# rtrn: nothing +# rtrn: Apache::OK on success, some other code on failure # efct: initializes the CODE field with the source script #########################################################################
# reads the contents of the file
sub read_script {
my $self = shift;
+ my $rc = Apache::OK; $self->debug("reading $self->{FILENAME}") if DEBUG & D_NOISE;
- $self->{CODE} = $self->{REQ}->slurp_filename(0); # untainted
+ $self->{CODE} = eval { $self->{REQ}->slurp_filename(0) }; # untainted
+ if ($@) {
+ $self->log_error("$@");
+ $rc = Apache::NOT_FOUND; # map various issues into one
+ }
+
+ return $rc;
}#########################################################################
-- __________________________________________________________________ Stas Bekman JAm_pH ------> Just Another mod_perl Hacker http://stason.org/ mod_perl Guide ---> http://perl.apache.org mailto:[EMAIL PROTECTED] http://use.perl.org http://apacheweek.com http://modperlbook.org http://apache.org http://ticketmaster.com
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
