Attachment 66318 Details for Bug 17989 – [SIGNED-OFF] Bug 17989: Extend bad template check

[patch] [SIGNED-OFF] Bug 17989: Extend bad template check

SIGNED-OFF-Bug-17989-Extend-bad-template-check.patch (text/plain), 4.13 KB, created by Josef Moravec on 2017-08-22 10:58:30 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Josef Moravec

Created: 2017-08-22 10:58:30 UTC

Size: 4.13 KB

patch

obsolete

>From bcfdc3e0334f4ea36318e03f576478e5f7710ce9 Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>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 <m.de.rooy@rijksmuseum.nl>
>
>Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
>---
> 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

Actions: View | Diff | Splinter Review

Attachments on bug 17989: 65277 | 65278 | 65279 | 65280 | 65281 | 66272 | 66273 | 66274 | 66275 | 66276 | 66317 | 66318 | 66319 | 66320 | 66321 | 67514 | 67515 | 67516 | 67517 | 67518 | 68654 | 68667 | 68668 | 68669 | 68670 | 68671 | 68672 | 68842 | 68843