perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Damon Buckwalter <buck...@gmail.com>
Subject Re: ACL filesystem incompatibility and potential race condition
Date Wed, 02 Mar 2005 07:38:20 GMT
On Tue, 01 Mar 2005 21:55:00 -0500, Stas Bekman <stas@stason.org> wrote:
> Damon Buckwalter wrote:
> > [originally posted to user list, apolgies for duplication]
> >
> > Greetings,
> >
> > I use mod_perl 2 on a Debian Linux system, from the Debian supplied
> > package.  I also use ext3 and jfs filesystems, which provide ACL
> > capabilites for assigning permissions.  In my particular
> > configuration, files are owned by my user and group, and not
> > world-readable.  In order for Apache (httpd) to read files to be
> > served, I assign an ACL giving the group that Apache runs as access to
> > read files (www-data on Debian).  In the process of doing this, I
> > noted that Apache serves files protected in such a manner without
> > incident, but mod_perl's Registry and PerlRun handlers refuse to.
> >
> > In an attempt to fix this problem a year ago, I worked to get a patch
> > added on Debian systems that will use the "use filetest 'access'"
> > pragma inside of RegistryCooker.  RegistryCooker must also be modified
> > to use $r->filename with the -r and -x filetests, since the "filetest
> > 'access'" pragma requires a filename, not stat() info.
> 
> Damon, please take a look at these threads:
> http://marc.theaimsgroup.com/?t=107623117600001&r=1&w=2
> http://marc.theaimsgroup.com/?t=107636627000001&r=1&w=2
> 
> Your patch is probably similar to the one Philippe has posted:
> http://marc.theaimsgroup.com/?l=apache-modperl-dev&m=107644764027510&w=2
> 
> The conclusion remains though: why penalizing users who don't use acls. A
> cooker subclass sounds like a more efficient solution for the general crowd.

Yes, I was involved in the development of this patch, by reporting the
bug to my Debian maintainer.  :^)  It is not an ideal solution, as it
provides different behaviours for ACL/non-ACL systems.  Also, it
mentions the possibility of a forked version of Registry just for
ACLs, which is unnecessary.

> 
> > Recently, I found that PerlRun was failing, even with the above patch.
> > This made me look closer at the code in RegistryCooker's
> > can_compile() function.  Why are we testing whether the file is
> > readable/executable before compilation, if this operation is not
> > executed atomically with the subsequent
> > ModPerl::Util::slurp_filename()/modperl_util.c:apr_file_open()?  This
> > seems to be insecure at worst and unreliable at best.
> 
> Not sure what do you find of insecure about it. first of all this is how
> mp1 registry was written, so mp2 is just a port of the existing code.
> second registry emulates mod_cgi, so all the checks run by mod_cgi are run
> by registry. If you are saying that those checks are insecure (later you
> mention non-atomicness), I believe mod_cgi does the same (I can double
> check if you want).

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(),

That said, I looked at the source for httpd-2.0.53 (and 2.1-dev) as
you suggest.  The code here to check the execute bit is commented out,
so it must have changed since your last recollection:

mod_cgi.c, about line 779
/*
    if (!ap_suexec_enabled) {
        if (!ap_can_exec(&r->finfo))
            return log_scripterror(r, conf, HTTP_FORBIDDEN, 0,
                                   "file permissions deny server execution");
    }

*/

I found this code mentioned (marginally at least) since December 2001:

http://marc.theaimsgroup.com/?l=apache-cvs&m=100826413926477&w=4

Later the code checks to see if the exec succeds or not, and
propagates an appropriate return value/HTTP status code.  I believe
that this is the right way to do it (Apahe seems to agree!), and
mod_perl should behave similarly.  mod_cgi works fine with ACLs
because of this logic.

> 
> Other than that I see no reason why we should require the exec bit on.
> Though it certainly needs to be readable, no?

Right, but the best way to find this out is to attempt to open the
file, not to check permission bits preemptively, just as mod_cgi does
not do anymore.

> 
> > I propose that the read/execute/directory tests be removed from
> > can_compile().  The slurp_filename() method should not assume success
> > as it does now, but instead return an error code indicating that a
> > file could not be opened/does not exist/is a directory/etc.
> 
> what made your think that slurp_filename doesn't handle errors properly?
> Take a look at: src/modules/perl/modperl_util.c:
> 
> MP_INLINE SV *modperl_slurp_filename(pTHX_ request_rec *r, int tainted)
> 
> In the mp2 API we do most error checks on behalf of the user. so in case
> something goes wrong, it will croak.

But checking preemptively does not match mod_cgi and causes problems
with ACLs.  It ought instead to behave in the spirit of mod_cgi as you
suggested before.  Attempt to open the file, and return appropriate
error/HTTP status from the return code of apr_file_open.  This would
create a common code path for ACL/non-ACL systems, with no extra
stress placed on either for compatibility with the other.

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. ;^)

Best Regards,
Damon

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


Mime
View raw message