From bcfdc3e0334f4ea36318e03f576478e5f7710ce9 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Mon, 30 Jan 2017 09:24:22 +0100 Subject: [PATCH] [SIGNED-OFF] Bug 17989: Extend bad template check The check is now extended by only allowing templates from the regular Koha template directories and additional plugin directories as defined in koha-conf.xml. Note: In order to prevent an uninitialized warning on $theme from sub themelanguage for a not-existing file, I added a trivial assignment. Will get further attention in a follow-up. Test plan: [1] Run t/db_dependent/Auth.t [2] Run t/db_dependent/Templates.t Signed-off-by: Marcel de Rooy Signed-off-by: Josef Moravec --- C4/Templates.pm | 19 +++++++++++++++++-- t/db_dependent/Templates.t | 18 +++++++++++++++--- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/C4/Templates.pm b/C4/Templates.pm index 94c5baf..0f9f007 100644 --- a/C4/Templates.pm +++ b/C4/Templates.pm @@ -163,6 +163,7 @@ sub _get_template_file { my $htdocs = C4::Context->config($is_intranet ? 'intrahtdocs' : 'opachtdocs'); my ($theme, $lang, $availablethemes) = themelanguage($htdocs, $tmplbase, $interface, $query); $lang //= 'en'; + $theme //= ''; my $filename = "$htdocs/$theme/$lang/modules/$tmplbase"; return ($htdocs, $theme, $lang, $filename); @@ -181,8 +182,22 @@ sub _get_template_file { sub badtemplatecheck { my ( $template ) = @_; - Koha::Exceptions::NoPermission->throw( 'bad template path' ) - unless $template =~ m/^[a-zA-Z0-9_\-\/]+\.(tt|pref)$/; + if( !$template || $template !~ m/^[a-zA-Z0-9_\-\/]+\.(tt|pref)$/ ) { + # This also includes two dots + Koha::Exceptions::NoPermission->throw( 'bad template path' ); + } else { + # Check allowed dirs + my $dirs = C4::Context->config("pluginsdir"); + $dirs = [ $dirs ] if !ref($dirs); + unshift @$dirs, C4::Context->config('opachtdocs'), C4::Context->config('intrahtdocs'); + my $found = 0; + foreach my $dir ( @$dirs ) { + $dir .= '/' if $dir !~ m/\/$/; + $found++ if $template =~ m/^$dir/; + last if $found; + } + Koha::Exceptions::NoPermission->throw( 'bad template path' ) if !$found; + } } sub gettemplate { diff --git a/t/db_dependent/Templates.t b/t/db_dependent/Templates.t index 760b9f0..1057ae5 100755 --- a/t/db_dependent/Templates.t +++ b/t/db_dependent/Templates.t @@ -97,11 +97,23 @@ subtest 'Testing themelanguage' => sub { return; }; -subtest 'Testing gettemplate' => sub { - plan tests => 2; +subtest 'Testing gettemplate/badtemplatecheck' => sub { + plan tests => 7; + my $cgi = CGI->new; my $template; - warning_like { eval { $template = C4::Templates::gettemplate( '/etc/passwd', 'opac', CGI->new, 1 ) }; warn $@ if $@; } qr/bad template/, 'Bad template check'; + warning_like { eval { $template = C4::Templates::gettemplate( '/etc/passwd', 'opac', $cgi, 1 ) }; warn $@ if $@; } qr/bad template/, 'Bad template check'; is( $template ? $template->output: '', '', 'Check output' ); + + # Test a few more bad paths to gettemplate triggering badtemplatecheck + warning_like { eval { C4::Templates::gettemplate( '../topsecret.tt', 'opac', $cgi, 1 ) }; warn $@ if $@; } qr/bad template/, 'No safe chars'; + warning_like { eval { C4::Templates::gettemplate( '/noaccess/topsecret.tt', 'opac', $cgi, 1 ) }; warn $@ if $@; } qr/bad template/, 'Directory not allowed'; + warning_like { eval { C4::Templates::gettemplate( C4::Context->config('intrahtdocs') . '2/prog/en/modules/about.tt', 'intranet', $cgi, 1 ) }; warn $@ if $@; } qr/bad template/, 'Directory not allowed too'; + + # Allow templates from /tmp + t::lib::Mocks::mock_config( 'pluginsdir', [ '/tmp' ] ); + warning_like { eval { C4::Templates::badtemplatecheck( '/tmp/about.tt' ) }; warn $@ if $@; } undef, 'No warn on template from plugin dir'; + # Refuse wrong extension + warning_like { eval { C4::Templates::badtemplatecheck( '/tmp/about.tmpl' ) }; warn $@ if $@; } qr/bad template/, 'Warn on bad extension'; }; -- 2.1.4