From 1bb1698f2a9a52f10b4dd0bac0afe8fc95546316 Mon Sep 17 00:00:00 2001 From: Erik Johansen Date: Tue, 29 Sep 2026 09:05:46 +0200 Subject: [PATCH] prevent storage ids from escaping the storage root Storage IDs were passed straight to Mojo::File->child, so IDs with `..` parts (e.g. `/storage/..%2F..%2Fetc%2Fpasswd`) and upload filenames like `../../x` could read, write, or delete files outside of the root. Add Yancy::Storage->valid_id, which rejects empty IDs, `.` and `..` parts, backslashes, and NUL characters, and check it in get, put, and delete. The storage controller returns 404/400 for invalid IDs, and only uses the file name part of an upload's filename. Also return undef from get when the file does not exist, so the controller can respond with 404. Co-Authored-By: Claude Opus 5.5 --- lib/Yancy/Controller/Storage.pm | 29 ++++++++++++++++++++---- lib/Yancy/Storage.pm | 40 ++++++++++++++++++++++++--------- t/controller/storage.t | 32 ++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 14 deletions(-) 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;