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 17:36:07 GMT
On Wed, 02 Mar 2005 08:10:03 -0500, Stas Bekman <stas@stason.org> wrote:
> Damon Buckwalter wrote:
> > On Tue, 01 Mar 2005 21:55:00 -0500, Stas Bekman <stas@stason.org> 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.

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

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.

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

As I said before, I am most comfortable with the Perl bits, but I will
try to make at least a working example of my concept in the C/XS code.

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

Right, I was talking about can_compile again here, sorry.

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

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


Mime
View raw message