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 23:32:40 GMT
Damon Buckwalter wrote:
> 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.

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

Please take a look at the attached patch. I believe it does what you want. 
The only remaining concern of mine is the -x bit.

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

No worries, Damon, you are doing a good job.

Here is the patch (3 sub tests now fail since we no longer report 
FORBIDDEN when -x is not there):

Index: ModPerl-Registry/lib/ModPerl/RegistryCooker.pm
===================================================================
--- ModPerl-Registry/lib/ModPerl/RegistryCooker.pm	(revision 155373)
+++ ModPerl-Registry/lib/ModPerl/RegistryCooker.pm	(working copy)
@@ -254,21 +254,10 @@
      my $self = shift;
      my $r = $self->{REQ};

-    unless (-r $r->my_finfo && -s _) {
-        $self->log_error("$self->{FILENAME} not found or unable to stat");
-        return Apache::NOT_FOUND;
-    }
+    return Apache::DECLINED if -d $r->my_finfo;

-    return Apache::DECLINED if -d _;
-
      $self->{MTIME} = -M _;

-    unless (-x _ or IS_WIN32) {
-        $r->log_error("file permissions deny server execution",
-                       $self->{FILENAME});
-        return Apache::FORBIDDEN;
-    }
-
      if (!($r->allow_options & Apache::OPT_EXECCGI)) {
          $r->log_error("Options ExecCGI is off in this directory",
                         $self->{FILENAME});
@@ -372,10 +361,13 @@
  sub convert_script_to_compiled_handler {
      my $self = shift;

+    my $rc = Apache::OK;
+
      $self->debug("Adding package $self->{PACKAGE}") if DEBUG & D_NOISE;

      # get the script's source
-    $self->read_script;
+    $rc = $self->read_script;
+    return $rc unless $rc == Apache::OK;

      # convert the shebang line opts into perl code
      $self->rewrite_shebang;
@@ -408,7 +400,7 @@
                      ${ $self->{CODE} },
                      "\n}"; # last line comment without newline?

-    my $rc = $self->compile(\$eval);
+    $rc = $self->compile(\$eval);
      return $rc unless $rc == Apache::OK;
      $self->debug(qq{compiled package \"$self->{PACKAGE}\"}) if DEBUG & 
D_NOISE;

@@ -534,16 +526,23 @@
  # dflt: read_script
  # desc: reads the script in
  # args: $self - registry blessed object
-# rtrn: nothing
+# rtrn: Apache::OK on success, some other code on failure
  # efct: initializes the CODE field with the source script
  #########################################################################

  # reads the contents of the file
  sub read_script {
      my $self = shift;
+    my $rc = Apache::OK;

      $self->debug("reading $self->{FILENAME}") if DEBUG & D_NOISE;
-    $self->{CODE} = $self->{REQ}->slurp_filename(0); # untainted
+    $self->{CODE} = eval { $self->{REQ}->slurp_filename(0) }; # untainted
+    if ($@) {
+        $self->log_error("$@");
+        $rc = Apache::NOT_FOUND; # map various issues into one
+    }
+
+    return $rc;
  }

  #########################################################################

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