Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plack::App::File prunes trailing slashes via split() invocation #405

Closed
avar opened this issue May 28, 2013 · 5 comments
Closed

Plack::App::File prunes trailing slashes via split() invocation #405

avar opened this issue May 28, 2013 · 5 comments

Comments

@avar
Copy link
Contributor

avar commented May 28, 2013

I had to deal with a security issue/bug that arose due to an
interaction with Plack::App::File which was basically:

  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

The problem is that if you request:

/file.txt.pl/

You'll get the file at:

/file.txt.pl

But without it having gone throug step #2, it just gets served up as a plain file!

The reason for this is that Plack::App::File does a split on "/"
without having a third -1 argument. So e.g. this:

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

Is the same as:

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

As opposed to:

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

I don't think the magical split behavior of ignoring empty trailing
fields has any place in the implicit Plack::App::File API. But
opinions may differ, so filing this bug.

Note the difference in behavior v.s. standard *nix utilities:

$ 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)

If touch behaved like Plack::App::File it would happily ignore the
trailing slash.

@miyagawa
Copy link
Member

Good find - can you make a unit test that represents the bug?

avar added a commit to avar/Plack that referenced this issue Feb 7, 2014
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 plack#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;
        }
    }
@sander85
Copy link

Hi!

Can someone explain me how to test this fix? I have 1.0031 installed but when I run

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

then the output is still this:

$VAR1 = [
          'a',
          'file.txt'
        ];

What am I missing?

@avar
Copy link
Contributor Author

avar commented Nov 23, 2014

That's not what that command outputs, it emits this:

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

What shell/OS are you running this on, you should be getting something like this in a unix shell:

$ perl -MO=Deparse -MData::Dumper -wle 'print Dumper [split /[\\\/]/, shift, -1]' a/file.txt///
BEGIN { $^W = 1; }
BEGIN { $/ = "\n"; $\ = "\n"; }
use Data::Dumper;
print Dumper([split(m[[\\/]], shift @ARGV, -1)]);
-e syntax OK

@sander85
Copy link

Oh, my bad. I used the wrong command :/ Sorry about that!

But is there any POC for this fix? I mean, how can QA test that the issue is fixed? :)

@miyagawa
Copy link
Member

The verification for fix is in the test: https://github.com/plack/Plack/pull/446/files#diff-2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants