From 11bf7e7bef856d5d90126c19f897d060cb4c9d9d Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Date: Thu, 18 Aug 2016 15:52:38 +0100
Subject: [PATCH] Bug 17146: Fix CSRF in picture-upload.pl

If an attacker can get an authenticated Koha user to visit their page
with the
url below, they can change or delete patrons' images
/tools/picture-upload.pl?op=Delete&borrowernumber=42

Test plan:
1/ Hit /tools/picture-upload.pl?op=Delete&borrowernumber=42
And confirm that you get a "Wrong CSRF token" error
2/ Go on the patron detail page with a patron's image
3/ Click on the Delete link (note the csrf_token param)
4/ The image will be deleted and you are redirected to the patron detail
page.

Regression tests:
Upload an image from the patron detail page and from the "upload patron
images" tool.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
---
 .../prog/en/modules/members/moremember.tt        |  3 ++-
 .../prog/en/modules/tools/picture-upload.tt      |  1 +
 members/moremember.pl                            | 10 ++++++++++
 tools/picture-upload.pl                          | 16 ++++++++++++++++
 4 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
index 8212451d5d..879f094179 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
@@ -305,9 +305,10 @@ function validate1(date) {
                     </li>
                 </ol>
                 <fieldset class="action">
+                    <input type="hidden" name="csrf_token" value="[% csrf_token %]" />
                     <input type="submit" value="Upload" class="submit" />
                     <input name="op" type="hidden" value="Upload" />
-                    [% IF ( picture ) %]<a id="delpicture" href="/cgi-bin/koha/tools/picture-upload.pl?op=Delete&amp;borrowernumber=[% borrowernumber %]" class="delete">Delete</a>[% END %]
+                    [% IF ( picture ) %]<a id="delpicture" href="/cgi-bin/koha/tools/picture-upload.pl?op=Delete&amp;borrowernumber=[% borrowernumber %]&amp;csrf_token=[% csrf_token %]" class="delete">Delete</a>[% END %]
                 </fieldset>
             </fieldset>
         </form>
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/picture-upload.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/picture-upload.tt
index 28fce72aad..958f810a7c 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/picture-upload.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/picture-upload.tt
@@ -143,6 +143,7 @@
                         </ol>
 	            </fieldset>
                     <fieldset class="action">
+                        <input type="hidden" name="csrf_token" value="[% csrf_token %]" />
                         <input type="hidden" name="op" value="Upload" />
                         <input type="submit" value="Upload" class="submit" />
 						<a href="/cgi-bin/koha/tools/tools-home.pl" class="cancel">Cancel</a>
diff --git a/members/moremember.pl b/members/moremember.pl
index 9f3ae74bdb..00e26006ce 100755
--- a/members/moremember.pl
+++ b/members/moremember.pl
@@ -36,6 +36,7 @@
 use strict;
 #use warnings; FIXME - Bug 2505
 use CGI qw ( -utf8 );
+use Digest::MD5 qw(md5_base64);
 use C4::Context;
 use C4::Auth;
 use C4::Output;
@@ -62,6 +63,7 @@ use DateTime;
 use Koha::DateUtils;
 use Koha::Database;
 use Koha::Patron::Categories;
+use Koha::Token;
 
 use vars qw($debug);
 
@@ -268,6 +270,14 @@ if ( C4::Context->preference('NorwegianPatronDBEnable') && C4::Context->preferen
 # patronimage related interface on
 my $patron_image = Koha::Patron::Images->find($data->{borrowernumber});
 $template->param( picture => 1 ) if $patron_image;
+# Generate CSRF token for upload and delete image buttons
+$template->param(
+    csrf_token => Koha::Token->new->generate_csrf({
+        id     => C4::Context->userenv->{id},
+        secret => md5_base64( C4::Context->config('pass') ),
+    }),
+);
+
 
 $template->param(%$data);
 
diff --git a/tools/picture-upload.pl b/tools/picture-upload.pl
index 9892eb5d17..6aeed8ace3 100755
--- a/tools/picture-upload.pl
+++ b/tools/picture-upload.pl
@@ -25,6 +25,7 @@ use File::Temp;
 use File::Copy;
 use CGI qw ( -utf8 );
 use GD;
+use Digest::MD5 qw(md5_base64);
 use C4::Context;
 use C4::Auth;
 use C4::Output;
@@ -34,6 +35,7 @@ use C4::Debug;
 use Koha::Patrons;
 use Koha::Patron::Image;
 use Koha::Patron::Images;
+use Koha::Token;
 
 my $input = new CGI;
 
@@ -82,6 +84,14 @@ our %errors = ();
 
 # Case is important in these operational values as the template must use case to be visually pleasing!
 if ( ( $op eq 'Upload' ) && $uploadfile ) {
+
+    die "Wrong CSRF token"
+        unless Koha::Token->new->check_csrf({
+            id     => C4::Context->userenv->{id},
+            secret => md5_base64( C4::Context->config('pass') ),
+            token  => scalar $input->param('csrf_token'),
+        });
+
     my $dirname = File::Temp::tempdir( CLEANUP => 1 );
     $debug and warn "dirname = $dirname";
     my $filesuffix;
@@ -175,6 +185,12 @@ if ( $borrowernumber && !%errors && !$template->param('ERRORS') ) {
         "/cgi-bin/koha/members/moremember.pl?borrowernumber=$borrowernumber");
 }
 else {
+    $template->param(
+        csrf_token => Koha::Token->new->generate_csrf({
+            id     => C4::Context->userenv->{id},
+            secret => md5_base64( C4::Context->config('pass') ),
+        }),
+    );
     output_html_with_http_headers $input, $cookie, $template->output;
 }