diff --git a/lib/Yancy/Controller/Storage.pm b/lib/Yancy/Controller/Storage.pm index ddfb11aa..742e841e 100644 --- a/lib/Yancy/Controller/Storage.pm +++ b/lib/Yancy/Controller/Storage.pm @@ -19,6 +19,10 @@ L, L use Mojo::Base 'Mojolicious::Controller', -signatures; +sub _invalid_id($c, $id) { + return $c->render( status => 400, json => { errors => [ { message => qq{Invalid storage ID "$id"} } ] } ); +} + sub _storage($c) { return $c->stash('storage') || $c->can('storage')->($c) || die "No storage?"; } @@ -28,7 +32,10 @@ sub _storage($c) { =cut sub get( $c ) { - my $asset = $c->_storage->get($c->param('id')) or return $c->reply->not_found; + my $storage = $c->_storage; + my $id = $c->param('id'); + return $c->reply->not_found unless $storage->valid_id($id); + my $asset = $storage->get($id) or return $c->reply->not_found; return $c->reply->asset($asset); } @@ -37,7 +44,10 @@ sub get( $c ) { =cut sub put( $c ) { - $c->_storage->put( $c->param('id'), $c->req->body ); + my $storage = $c->_storage; + my $id = $c->param('id'); + return $c->_invalid_id($id) unless $storage->valid_id($id); + $storage->put( $id, $c->req->body ); $c->rendered(201); } @@ -48,8 +58,16 @@ sub put( $c ) { sub post( $c ) { my $storage = $c->_storage; my $prefix = $c->param('prefix'); + my @uploads; for my $upload ( @{ $c->req->uploads } ) { - my $id = $prefix ? join( '/', $prefix, $upload->filename ) : $upload->filename; + # Browsers may send a full client path, so only keep the file name + my ( $name ) = $upload->filename =~ m{([^/\\]*)$}; + my $id = $prefix ? join( '/', $prefix, $name ) : $name; + return $c->_invalid_id($id) unless $storage->valid_id($id); + push @uploads, [ $id, $upload ]; + } + for my $item ( @uploads ) { + my ( $id, $upload ) = @$item; $c->log->info("Uploading file $id"); $storage->put( $id, $upload->asset ); } @@ -61,7 +79,10 @@ sub post( $c ) { =cut sub delete( $c ) { - $c->_storage->delete($c->param('id')); + my $storage = $c->_storage; + my $id = $c->param('id'); + return $c->_invalid_id($id) unless $storage->valid_id($id); + $storage->delete($id); return $c->rendered(201); } diff --git a/lib/Yancy/Storage.pm b/lib/Yancy/Storage.pm index 1553d7d7..8fb8a629 100644 --- a/lib/Yancy/Storage.pm +++ b/lib/Yancy/Storage.pm @@ -62,12 +62,15 @@ sub new($class, @attrs) { =method get -Get a file's content. Returns a L to serve. +Get a file's content. Returns a L to serve, or C +if the file does not exist. Dies if the ID is not L. =cut sub get( $self, $id ) { - return Mojo::Asset::File->new( path => $self->root->child( $id ) ); + my $file = $self->_file( $id ); + return undef unless -f $file; + return Mojo::Asset::File->new( path => $file ); } =method put @@ -78,13 +81,10 @@ L object. Returns the storage ID of the uploaded file. =cut sub put( $self, $id, $asset ) { - my @path_parts = grep $_, split m{/}, $id; - my $name = pop @path_parts; - my $root = $self->root; - my $path = $root->child( @path_parts )->make_path; - my $file_path = $path->child( $name ); - $file_path->spurt( blessed $asset ? $asset->slurp : $asset ); - return join '/', @path_parts, $name; + my $file_path = $self->_file( $id ); + $file_path->dirname->make_path; + $file_path->spew( blessed $asset ? $asset->slurp : $asset ); + return $file_path->to_rel( $self->root )->to_string; } =method delete @@ -95,7 +95,27 @@ was successfully deleted. =cut sub delete( $self, $id ) { - unlink $self->root->child($id); + unlink $self->_file( $id ); +} + +=method valid_id + +Returns true if the given storage ID is safe to use. Storage IDs must not be +empty and must not contain C<.> or C<..> path parts, backslashes, or NUL +characters, so they cannot refer to files outside of the L. + +=cut + +sub valid_id( $self, $id ) { + return 0 if !defined $id || $id =~ m{[\\\0]}; + my @parts = grep { length } split m{/}, $id; + return 0 if !@parts || grep { $_ eq '.' || $_ eq '..' } @parts; + return 1; +} + +sub _file( $self, $id ) { + die qq{Invalid storage ID "$id"\n} unless $self->valid_id( $id ); + return $self->root->child( grep { length } split m{/}, $id ); } =method list diff --git a/t/controller/storage.t b/t/controller/storage.t index 8e1065ec..25151730 100644 --- a/t/controller/storage.t +++ b/t/controller/storage.t @@ -71,4 +71,36 @@ subtest 'list' => sub { $t->get_ok('/storage')->status_is(200)->json_is([qw( foo/bar foobar )]); }; +subtest 'get missing file' => sub { + my ($t, $tempdir) = build_app(); + $t->get_ok('/storage/missing')->status_is(404); +}; + +subtest 'ids outside of root are rejected' => sub { + my ($t, $tempdir) = build_app(); + my $outside_name = 'yancy-storage-test-' . $$; + my $outside = $tempdir->dirname->child($outside_name); + $outside->spew('SECRET'); + + for my $id ( "..%2F$outside_name", "foo%2F..%2F..%2F$outside_name", '.', 'foo%5C..%5Cbar' ) { + $t->get_ok("/storage/$id")->status_is(404, "get $id"); + $t->put_ok("/storage/$id", 'PWNED')->status_is(400, "put $id"); + $t->delete_ok("/storage/$id")->status_is(400, "delete $id"); + } + is $outside->slurp, 'SECRET', 'file outside root not changed'; + + $t->post_ok('/storage/', form => { foo => { content => 'PWNED', filename => "../$outside_name" }}) + ->status_is(201); + is $outside->slurp, 'SECRET', 'upload path in filename is ignored'; + is $tempdir->child($outside_name)->slurp, 'PWNED', 'upload stored in root'; + + $t->post_ok('/storage/..', form => { foo => { content => 'PWNED', filename => 'x' }}) + ->status_is(400, 'upload prefix outside root'); + $t->post_ok('/storage/', form => { foo => { content => 'PWNED', filename => '..' }}) + ->status_is(400, 'upload filename ..'); + ok !-e $tempdir->dirname->child('x'), 'no upload outside root'; + + $outside->remove; +}; + done_testing;