Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Plack::App::File: Fix a security issue by not pruning trailing slashes #446

Merged
merged 1 commit into from

3 participants

@avar

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;
    }
}
@avar avar 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;
        }
    }
bc1731d
@coveralls

Coverage Status

Coverage decreased (-0.09%) when pulling bc1731d on avar:avar/fix-issue-405 into 9c8a5bc on plack:master.

@miyagawa miyagawa merged commit 458eb93 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 7, 2014
  1. @avar

    Plack::App::File: Fix a security issue by not pruning trailing slashes

    avar authored
    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;
            }
        }
This page is out of date. Refresh to see the latest.
Showing with 31 additions and 1 deletion.
  1. +11 −0 Changes
  2. +1 −1  lib/Plack/App/File.pm
  3. +19 −0 t/Plack-Middleware/file.t
View
11 Changes
@@ -1,6 +1,17 @@
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]
View
2  lib/Plack/App/File.pm
@@ -44,7 +44,7 @@ sub locate_file {
}
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 {
View
19 t/Plack-Middleware/file.t
@@ -3,6 +3,7 @@ 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');
@@ -35,6 +36,24 @@ test_psgi $app_content_type, sub {
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;
Something went wrong with that request. Please try again.