CVE-2014-5269: Permissions, Privileges, and Access Controls (CWE-264)
value: Plack::App::File in Plack before 1.0031 removes trailing slash characters from paths, which allows remote attackers to bypass the whitelist of generated files and obtain sensitive information via a crafted path, related to Plack::Middleware::Static.
CWE description: Weaknesses in this category are related to the management of permissions, privileges, and other security features that are used to perform access control.
Published: 2014-09-04T17:55Z
Repository: https://github.com/avar/Plack
Language: Other
Files changed: Changes, lib/Plack/App/File.pm, t/Plack-Middleware/file.t
Commit date: 2014-02-07 10:30:03 +0000
Commit message: Plack::App::File: Fix a security issue by not pruning trailing slashes

Before this Plack::App::File would prune trailing slashes via its split
invocation. I.e. it would think this:

    $ perl -MData::Dumper -wle 'print Dumper [split /[\\\/]/, shift]' a/file.txt
    $VAR1 = [
              'a',
              'file.txt'
            ];

Was the same as:

    $ perl -MData::Dumper -wle 'print Dumper [split /[\\\/]/, shift]' a/file.txt///
    $VAR1 = [
              'a',
              'file.txt'
            ];

This can. turn into a nasty code exposure issue if you e.g. have an app
that basically does this:

    1. I'd do a regex /.txt.pl\z/ on a file to see if it was a text file
    2. If so, do magic to generate text file via perl
    3. Else it's not a /.txt.pl\z/ file, so it must be some other static
       file with a different extension
    4. Serve it up with Plack::Middleware::Static

This is also not how other webservers or Unix utilities work:

    $ touch /tmp/foo.txt
    $ file /tmp/foo.txt
    /tmp/foo.txt: empty
    $ file /tmp/foo.txt/
    /tmp/foo.txt/: ERROR: cannot open `/tmp/foo.txt/' (Not a directory)

This resolves issue #405 that I filed around 9 months ago. I was
previously working around it in my own code by doing:

    {
        # Let's see if someone's trying to be evil by
        # requesting e.g. /index.html/ instead of
        # /index.html. We don't want to fall through
        # and just serve up the raw content.
        my $plack_app_file = Plack::App::File->new({ root => PLACK_WEBSERVER_DOCUMENT_ROOT() });
        my ($file) = $plack_app_file->locate_file($env);
        if (
            # We'll get a reference if it's a full
            # Plack response. I.e. a 404 or whatever.
            ref $file ne 'ARRAY'
            and
            # WTF once we canonicalize the file and it
            # looks like a Mason handled path let's
            # not accept it, because we don't want to
            # serve up the raw unprocessed Mason page
            # via this hack.
            $file =~ $mason_handles_this_path_rx
        ) {
            TELL "Middleware::Static: Path <$path> request, doesn't match <$mason_handles_this_path_rx>, but actually resolves to it via resolved file <$file>" if DEBUG;
            # Tells our app to just serve up a
            # 400. Apache would do a 404 but I think
            # these requests are bad, so say so.
            $env->{$magic_marker_to_return_400} = 1;
            return;
        }
    }

Diff: diff --git a/Changes b/Changes
index 02fb109..5b5c78c 100644
--- a/Changes
+++ b/Changes
@@ -1,8 +1,19 @@
 Go to http://github.com/plack/Plack/issues for the roadmap and known issues.
 
 {{$NEXT}}
+    [SECURITY]
+        - Plack::App::File would previously strip trailing slashes off
+          provided paths.
+
+          This in combination with the common pattern of dynamically
+          generating some files in a tree and serving the rest up with
+          Plack::Middleware::Static could allow an attacker to bypass
+          a whitelist of generated files by just requesting
+          /file.disallowed/ instead of /file.disallowed, provided that
+          Plack::Middleware::Static was used for all paths except
+          those matching /\.disallowed$/
 
 1.0030  2013-11-23 08:54:01 CET
     [IMPROVEMENTS]
         - Middleware::LogDispatch stringifies objects (oalders) #427
         - Encode wide character strings as UTF-8 in HTTP::Server::PSGI #429
diff --git a/lib/Plack/App/File.pm b/lib/Plack/App/File.pm
index de97372..049cb26 100644
--- a/lib/Plack/App/File.pm
+++ b/lib/Plack/App/File.pm
@@ -42,11 +42,11 @@ sub locate_file {
     if ($path =~ /\0/) {
         return $self->return_400;
     }
 
     my $docroot = $self->root || ".";
-    my @path = split /[\\\/]/, $path;
+    my @path = split /[\\\/]/, $path, -1; # -1 *MUST* be here to avoid security issues!
     if (@path) {
         shift @path if $path[0] eq '';
     } else {
         @path = ('.');
     }
diff --git a/t/Plack-Middleware/file.t b/t/Plack-Middleware/file.t
index a90a72d..92e51f3 100644
--- a/t/Plack-Middleware/file.t
+++ b/t/Plack-Middleware/file.t
@@ -1,10 +1,11 @@
 use strict;
 use Plack::Test;
 use Test::More;
 use HTTP::Request::Common;
 use Plack::App::File;
+use FindBin qw($Bin);
 
 my $app = Plack::App::File->new(file => 'Changes');
 
 test_psgi $app, sub {
     my $cb = shift;
@@ -33,8 +34,26 @@ test_psgi $app_content_type, sub {
     $res = $cb->(GET "/whatever");
     is $res->content_type, 'text/x-changes';
     is $res->code, 200;
 };
 
+my $app_secure = Plack::App::File->new(root => $Bin);
 
+test_psgi $app_secure, sub {
+    my $cb = shift;
+
+    my $res = $cb->(GET "/file.t");
+    is $res->code, 200;
+    like $res->content, qr/We will find for this literal string/;
+
+    my $res = $cb->(GET "/../Plack-Middleware/file.t");
+    is $res->code, 403;
+    is $res->content, 'forbidden';
+
+    for my $i (1..100) {
+        $res = $cb->(GET "/file.t" . ("/" x $i));
+        is $res->code, 404;
+        is $res->content, 'not found';
+    }
+};
 
 done_testing;

Vulnerable code:     my @path = split /[\\\/]/, $path;
Fixed code:     [SECURITY]
        - Plack::App::File would previously strip trailing slashes off
          provided paths.

          This in combination with the common pattern of dynamically
          generating some files in a tree and serving the rest up with
          Plack::Middleware::Static could allow an attacker to bypass
          a whitelist of generated files by just requesting
          /file.disallowed/ instead of /file.disallowed, provided that
          Plack::Middleware::Static was used for all paths except
          those matching /\.disallowed$/
    my @path = split /[\\\/]/, $path, -1; # -1 *MUST* be here to avoid security issues!
use FindBin qw($Bin);
my $app_secure = Plack::App::File->new(root => $Bin);
test_psgi $app_secure, sub {
    my $cb = shift;

    my $res = $cb->(GET "/file.t");
    is $res->code, 200;
    like $res->content, qr/We will find for this literal string/;

    my $res = $cb->(GET "/../Plack-Middleware/file.t");
    is $res->code, 403;
    is $res->content, 'forbidden';

    for my $i (1..100) {
        $res = $cb->(GET "/file.t" . ("/" x $i));
        is $res->code, 404;
        is $res->content, 'not found';
    }
};