From ced17de570cb1fab9657d27719daede83c5b15e0 Mon Sep 17 00:00:00 2001 From: Sebastian Riedel Date: Mon, 23 Jan 2023 15:40:39 +0100 Subject: [PATCH 1/2] Fix missing input validation for needle API endpoints Progress: https://progress.opensuse.org/issues/123490 --- lib/OpenQA/WebAPI.pm | 4 ++-- lib/OpenQA/WebAPI/Controller/File.pm | 5 +++-- t/ui/07-file.t | 8 +++++--- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/OpenQA/WebAPI.pm b/lib/OpenQA/WebAPI.pm index 91d3c0416ea..d64179f7938 100644 --- a/lib/OpenQA/WebAPI.pm +++ b/lib/OpenQA/WebAPI.pm @@ -184,8 +184,8 @@ sub startup ($self) { $step_auth->post('/')->name('save_needle_ajax')->to('step#save_needle_ajax'); $step_r->get('/')->name('step')->to(action => 'view'); - $r->get('/needles/:needle_id/image')->name('needle_image_by_id')->to('file#needle_image_by_id'); - $r->get('/needles/:needle_id/json')->name('needle_json_by_id')->to('file#needle_json_by_id'); + $r->get('/needles//image')->name('needle_image_by_id')->to('file#needle_image_by_id'); + $r->get('/needles//json')->name('needle_json_by_id')->to('file#needle_json_by_id'); $r->get('/needles/:distri/#name')->name('needle_file')->to('file#needle'); # this route is used in the helper $r->get('/image/:md5_dirname/.thumbs/#md5_basename')->name('thumb_image')->to('file#thumb_image'); diff --git a/lib/OpenQA/WebAPI/Controller/File.pm b/lib/OpenQA/WebAPI/Controller/File.pm index d20375b3716..ae52f19be04 100644 --- a/lib/OpenQA/WebAPI/Controller/File.pm +++ b/lib/OpenQA/WebAPI/Controller/File.pm @@ -61,8 +61,9 @@ sub needle ($self) { } sub _needle_by_id_and_extension ($self, $extension) { - my $needle_id = $self->param('needle_id') or return $self->reply->not_found; - my $needle = $self->schema->resultset('Needles')->find($needle_id) or return $self->reply->not_found; + return $self->reply->not_found unless my $needle_id = $self->param('needle_id'); + return $self->reply->not_found unless my $needle = $self->schema->resultset('Needles')->find($needle_id); + my $needle_dir = $needle->directory->path; my $needle_filename = $needle->name . $extension; diff --git a/t/ui/07-file.t b/t/ui/07-file.t index b0eb6307157..643cefaf3ee 100644 --- a/t/ui/07-file.t +++ b/t/ui/07-file.t @@ -72,9 +72,11 @@ subtest 'needle download' => sub { $t->get_ok('/needles/1/image')->status_is(200)->content_type_is('image/png')->content_is("png\n"); $t->get_ok('/needles/1/json')->status_is(200)->content_type_is('application/json;charset=UTF-8')->content_is($json); - # arguably this should work and be tested, but does not work now because - # of how we do routing: - #$t->get_ok('/needles/opensuse/subdirectory/inst-subdirectory.png') + # broken requests we saw in production that should not get past input validation + $t->get_ok('/needles/root-console-20200501/image')->status_is(404); + $t->get_ok('/needles/root-console-20200501/json')->status_is(404); + + $t->get_ok('/needles/opensuse/subdirectory/inst-subdirectory.png'); # currently you can only find a needle in a subdirectory by passing the # jsonfile query parameter like this: From 68440d61a7f921dd462dc03d851e90cf2f3b387b Mon Sep 17 00:00:00 2001 From: Sebastian Riedel Date: Mon, 23 Jan 2023 16:32:46 +0100 Subject: [PATCH 2/2] Initialize Mojolicious::Static only in one place This avoids cases where the instance is accidentally recreated, or not created at all. --- lib/OpenQA/WebAPI/Controller/File.pm | 55 ++++++++++++++-------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/lib/OpenQA/WebAPI/Controller/File.pm b/lib/OpenQA/WebAPI/Controller/File.pm index ae52f19be04..0a78416c826 100644 --- a/lib/OpenQA/WebAPI/Controller/File.pm +++ b/lib/OpenQA/WebAPI/Controller/File.pm @@ -12,6 +12,9 @@ use File::Spec; use File::Spec::Functions 'catfile'; use Data::Dump 'pp'; use Mojo::File 'path'; +use Scalar::Util qw(blessed); + +has static => sub { Mojolicious::Static->new }; sub needle ($self) { # do the format splitting ourselves instead of using mojo to restrict the suffixes @@ -54,10 +57,10 @@ sub needle ($self) { # we got something like subdir/needle.json, $path will be "subdir" $needledir .= "/$path"; } - push(@{($self->{static} = Mojolicious::Static->new)->paths}, $needledir); + push @{$self->static->paths}, $needledir; # name is an URL parameter and can't contain slashes, so it should be safe - return $self->serve_static_($name . $format); + return $self->_serve_static($name . $format); } sub _needle_by_id_and_extension ($self, $extension) { @@ -67,9 +70,8 @@ sub _needle_by_id_and_extension ($self, $extension) { my $needle_dir = $needle->directory->path; my $needle_filename = $needle->name . $extension; - $self->{static} = Mojolicious::Static->new; - push(@{$self->{static}->paths}, $needle_dir); - return $self->serve_static_($needle_filename); + push @{$self->static->paths}, $needle_dir; + return $self->_serve_static($needle_filename); } sub needle_image_by_id ($self) { @@ -86,16 +88,13 @@ sub _set_test ($self) { $self->{testdirname} = $self->{job}->result_dir; return unless $self->{testdirname}; - $self->{static} = Mojolicious::Static->new; - push @{$self->{static}->paths}, $self->{testdirname}; - push @{$self->{static}->paths}, $self->{testdirname} . "/ulogs"; + push @{$self->static->paths}, $self->{testdirname}, "$self->{testdirname}/ulogs"; return 1; } sub test_file ($self) { return $self->reply->not_found unless $self->_set_test; - - return $self->serve_static_($self->param('filename')); + return $self->_serve_static($self->param('filename')); } sub download_asset ($self) { @@ -140,32 +139,33 @@ sub test_asset ($self) { return $self->redirect_to($path); } -sub serve_static_ ($self, $asset) { - $self->app->log->debug("looking for " . pp($asset) . " in " . pp($self->{static}->paths)); - $asset = $self->{static}->file($asset) if $asset && !ref($asset); +sub _serve_static ($self, $asset) { + my $static = $self->static; + my $log = $self->log; + + $log->debug('looking for ' . pp($asset) . ' in ' . pp($static->paths)); + $asset = $static->file($asset) if $asset && !ref($asset); return $self->reply->not_found unless $asset; - $self->app->log->debug("found " . pp($asset)); + $log->debug('found ' . pp($asset)); - if (ref($asset) eq "Mojo::Asset::File") { + if (blessed $asset && $asset->isa('Mojo::Asset::File')) { my $filename = basename($asset->path); # guess content type from extension + my $headers = $self->res->headers; if ($filename =~ m/\.([^\.]+)$/) { my $ext = $1; my $filetype = $self->app->types->type($ext); - if ($filetype) { - $self->res->headers->content_type($filetype); - } - if ($ext eq 'iso') { - # force saveAs - $self->res->headers->content_disposition("attachment; filename=$filename;"); - } + $headers->content_type($filetype) if $filetype; + + # force saveAs + $headers->content_disposition("attachment; filename=$filename;") if $ext eq 'iso'; } else { $self->res->headers->content_type("application/octet-stream"); } } - $self->{static}->serve_asset($self, $asset); + $static->serve_asset($self, $asset); return !!$self->rendered; } @@ -173,18 +173,17 @@ sub serve_static_ ($self, $asset) { sub test_thumbnail ($self) { return $self->reply->not_found unless $self->_set_test; - my $asset = $self->{static}->file(".thumbs/" . $self->param('filename')); - return $self->serve_static_($asset); + my $asset = $self->static->file(".thumbs/" . $self->param('filename')); + return $self->_serve_static($asset); } # this is the agnostic route to images - usually served by apache directly sub thumb_image ($self) { - $self->{static} = Mojolicious::Static->new; - push @{$self->{static}->paths}, imagesdir(); + push @{$self->static->paths}, imagesdir(); # name is an URL parameter and can't contain slashes, so it should be safe my $dir = $self->param('md5_dirname') || ($self->param('md5_1') . '/' . $self->param('md5_2')); - return $self->serve_static_("$dir/.thumbs/" . $self->param('md5_basename')); + return $self->_serve_static("$dir/.thumbs/" . $self->param('md5_basename')); } 1;