Bug 17097: Fix CSRF in deletemem.pl

If an attacker can get an authenticated Koha user to visit their page
with the url below, they can delete patrons details.

  /members/deletemem.pl?member=42

Test plan:

0/ Do not apply any patches
1/ Adapt and hit the url above
=> The patron will be deleted without confirmation
2/ Apply first patch
3/ Hit the url
=> you will get a confirmation page
4/ Hit /members/deletemem.pl?member=42&delete_confirmed=1
=> The patron will be deleted without confirmation
5/ Apply the second patch (this one)
6/ Hit /members/deletemem.pl?member=42&delete_confirmed=1
=> you will get a crash "Wrong CSRF token" (no need to stylish)
7/ Delete a patron from the detail page and confirm the deletion
=> you will be redirected to the patron module home page and the patron
has been deleted

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>

diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/deletemem.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/deletemem.tt
index 9e24bd6..cd5902d 100644 (file)
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/deletemem.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/deletemem.tt
@@ -33,6 +33,7 @@
         <div class="dialog alert">
             <h3>Are you sure you want to delete the patron [% firstname %] [% surname %]? This cannot be undone.</h3>
             <form action="/cgi-bin/koha/members/deletemem.pl">
+                <input type="hidden" name="csrf_token" value="[% csrf_token %]" />
                 <input type="hidden" name="borrowernumber" value="[% borrowernumber %]"/>
                 <input type="hidden" name="op" value="delete_confirmed" />
                 <button type="submit" class="approve"><i class="fa fa-fw fa-check"></i> Yes, delete</button>

diff --git a/members/deletemem.pl b/members/deletemem.pl
index 173d70e..8da16a4 100755 (executable)
--- a/members/deletemem.pl
+++ b/members/deletemem.pl
@@ -25,6 +25,7 @@ use strict;
 #use warnings; FIXME - Bug 2505
 
 use CGI qw ( -utf8 );
+use Digest::MD5 qw(md5_base64);
 use C4::Context;
 use C4::Output;
 use C4::Auth;
@@ -32,6 +33,8 @@ use C4::Members;
 use C4::Branch; # GetBranches
 use Module::Load;
 use Koha::Patron::Images;
+use Koha::Token;
+
 if ( C4::Context->preference('NorwegianPatronDBEnable') && C4::Context->preference('NorwegianPatronDBEnable') == 1 ) {
     load Koha::NorwegianPatronDB, qw( NLMarkForDeletion NLSync );
 }
@@ -142,9 +145,23 @@ if ( $op eq 'delete_confirm' or $countissues > 0 or $flags->{'CHARGES'}  or $is_
     }
     # This is silly written but reflect the same conditions as above
     if ( not $countissues > 0 and not $flags->{CHARGES} ne '' and not $is_guarantor and not $deletelocal == 0 ) {
-        $template->param( op => 'delete_confirm' );
+        $template->param(
+            op         => 'delete_confirm',
+            csrf_token => Koha::Token->new->generate_csrf(
+                {   id     => C4::Context->userenv->{id},
+                    secret => md5_base64( C4::Context->config('pass') ),
+                }
+            ),
+        );
     }
 }elsif ( $op eq 'delete_confirmed' ) {
+
+    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'),
+        });
     MoveMemberToDeleted($member);
     C4::Members::HandleDelBorrower($member);
     DelMember($member);