From fcf38896bd738767c3a9c4c1e8909a199f480a30 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 9 Aug 2016 22:29:25 +0100 Subject: [PATCH] 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 Signed-off-by: Marcel de Rooy Signed-off-by: Kyle M Hall --- .../prog/en/modules/members/deletemem.tt | 1 + members/deletemem.pl | 19 ++++++++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) 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 9e24bd6a46..cd5902d087 100644 --- 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 @@

Are you sure you want to delete the patron [% firstname %] [% surname %]? This cannot be undone.

+ diff --git a/members/deletemem.pl b/members/deletemem.pl index 173d70e334..8da16a4dc1 100755 --- 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); -- 2.39.5