perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stas Bekman <s...@stason.org>
Subject Re: ACL filesystem incompatibility and potential race condition
Date Thu, 03 Mar 2005 02:06:18 GMT
Damon Buckwalter wrote:
> On Wed, 02 Mar 2005 18:32:40 -0500, Stas Bekman <stas@stason.org> wrote:
> 
>>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'd say no, because we're not really exec'ing anything.  It will be a
> user education issue, that PerlRun/Registry scripts will run if you
> tell Apache to handle then and ExecCGI is enabled in that context. 
> Plus it breaks my whole scheme to make mod_perl ACL compatible...

That's fine with me. Unless other developers disagree we can go with it.
[...]

> I managed to come up with something very similar to this, minus the
> last bit.  I like it, but I did manage to (partially) implement some
> code on the C/XS side to return different statuses based on whether
> the file is unreadable or non-existent.  It is broken because I can't
> figure out how to return the script from modperl_slurp_filehandle() to
> $self->{CODE}.  I just started learning XS today ;^)...  I was able to
> make missing or unreadable files return a proper status code at least.

You don't need to do any XS changes. Just the following will do:

     $self->{CODE} = eval { $self->{REQ}->slurp_filename(0) }; # untainted
     if ($@) {
         $self->log_error("$@");
         $rc = $@ == APR::EACCES ? Apache::FORBIDDEN : Apache::NOT_FOUND;
     }

$@ is a magic object under mod_perl 2. Take a look at:
http://perl.apache.org/docs/2.0/api/APR/Error.html

Your case is certainly faulty:

+    if ($rc == APR::EACCES) {
+        return Apache::DECLINED;

why declined and not forbidden?

+    } elsif ($rc == APR::ENOENT) {
+        return Apache::NOT_FOUND;
+    } else {
+        return Apache::OK;

why default to Apache::OK, which may miss some other errors you didn't 
encounter for. We should either default to Apache::NOT_FOUND or 500, but 
see below.

Further APR::EACCES will be given no matter whether the file exists or its 
perms are not right. So you will never end up with Apache::NOT_FOUND.

You can see that tests like t/404.t are failing with it, when using this 
version in addition to my last patch:

sub read_script {
     my $self = shift;
     my $rc = Apache::OK;

     $self->debug("reading $self->{FILENAME}") if DEBUG & D_NOISE;
     $self->{CODE} = eval { $self->{REQ}->slurp_filename(0) }; # untainted
     if ($@) {
         $rc = $@ == APR::EACCES ? Apache::FORBIDDEN : Apache::NOT_FOUND;
         $self->log_error("$@");
     }

     return $rc;
}

I'd rather always return Apache::NOT_FOUND, and log_error() logs the right 
error message. I don't think the error code makes any difference to the 
user, when they can't reach the page.

regarding changing the slurp_filename() API: passing a buffer by reference 
is ugly and not perlish (though we do it sometimes where we have no 
choice). Here $@ gives us the exact error code and stringified error 
message if wanted. Ideally it should have been $!, but unfortunately it's 
impossible to extend perl-core functionality on this one. Hence the $@ object.


-- 
__________________________________________________________________
Stas Bekman            JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/     mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org   http://ticketmaster.com

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org


Mime
View raw message