On Tue, 01 Mar 2005 21:55:00 -0500, Stas Bekman <[EMAIL PROTECTED]> 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:[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]
