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 Wed, 02 Mar 2005 13:10:03 GMT
Damon Buckwalter wrote:
> 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.

Agreed if it doesn't penalize other Registry users.

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

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.

> 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.

So by dropping that -x check, we no longer need a special case for acl fs, 
right? All is needed is to drop it.

>>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.

sure, we can do that. So you have a patch to solve this, right?

>>>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.

If you are talking about slurp_filename, it does not do a preemptive check:

#define SLURP_SUCCESS(action) \
     if (rc != APR_SUCCESS) { \
         SvREFCNT_dec(sv); \
         Perl_croak(aTHX_ "Error " action " '%s': %s ", r->filename, \
                    modperl_error_strerror(aTHX_ rc)); \
     }

     rc = apr_file_open(&file, r->filename, APR_READ|APR_BINARY,
                        APR_OS_DEFAULT, r->pool);
     SLURP_SUCCESS("opening");

     rc = apr_file_read(file, SvPVX(sv), &size);
     SLURP_SUCCESS("reading");

> 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 :)


-- 
__________________________________________________________________
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