Skip to content

Commit

Permalink
Resolve relative paths in send_file:
Browse files Browse the repository at this point in the history
If someone were to send a file to send_file() that includes '../',
then we would allow them to reach outside the directory we choose.
This is a possible security issue. (One can argue the user should
sanitize their input, but I think we simply shouldn't allow it.)

The problem is that Path::Tiny does the right thing and allows us to
reach there. To prevent that, we're resolving paths using
Path::Tiny's realpath() method and then subsumes() to see that the
file is within the original directory.

Otherwise, we send a 403 forbidden. There is also a test that verifies
this is done correctly.
  • Loading branch information
xsawyerx committed Sep 8, 2019
1 parent 3f293c6 commit 92d28f6
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 1 deletion.
8 changes: 7 additions & 1 deletion lib/Dancer2/Core/App.pm
Original file line number Diff line number Diff line change
Expand Up @@ -1044,8 +1044,14 @@ sub send_file {
$self->with_return->( $self->response );
};

$file_path = Path::Tiny::path( $dir, $path );
# resolve relative paths (with '../') as much as possible
$file_path = Path::Tiny::path( $dir, $path )->realpath;

# We need to check whether they are trying to access
# a directory outside their scope
$err_response->(403) if !Path::Tiny::path($dir)->realpath->subsumes($file_path);

# other error checks
$err_response->(403) if !$file_path->exists;
$err_response->(404) if !$file_path->is_file;
$err_response->(403) if !-r $file_path;
Expand Down
15 changes: 15 additions & 0 deletions t/dsl/send_file.t
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ use Ref::Util qw<is_coderef>;
send_file 'index.html';
};

get '/illegal' => sub {
send_file '../index.html';
};

prefix '/some' => sub {
get '/image' => sub {
send_file '1x1.png';
Expand Down Expand Up @@ -144,6 +148,17 @@ test_psgi $app, sub {
ok($r->is_success, 'send_file returns success');
is($r->header('Content-Disposition'), 'inline; filename="1x1.png"', 'send_file returns correct inline Content-Disposition');
};

subtest "Illegal path" => sub {
my $r = $cb->( GET '/illegal' );
is( $r->code, 403, 'Illegal path returns 403' );
is(
$r->content,
'Forbidden',
'Text content contains UTF-8 characters',
);
};

};

done_testing;

0 comments on commit 92d28f6

Please sign in to comment.