prevent storage ids from escaping the storage root

lib/Yancy/Controller/Storage.pm

@@ -19,6 +19,10 @@ L<Yancy::Storage>, L<Yancy::Plugin::Storage>
 
 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);
 }
 

lib/Yancy/Storage.pm

@@ -62,12 +62,15 @@ sub new($class, @attrs) {
 
 =method get
 
-Get a file's content. Returns a L<Mojo::Asset> to serve.
+Get a file's content. Returns a L<Mojo::Asset> to serve, or C<undef>
+if the file does not exist. Dies if the ID is not L<valid|/valid_id>.
 
 =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<Mojo::Asset> 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</root>.
+
+=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

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;