Attachment 171543 Details for Bug 37786 – Bug 37786: members/cancel-charge.pl needs CSRF protection

[patch] Bug 37786: members/cancel-charge.pl needs CSRF protection

Bug-37786-memberscancel-chargepl-needs-CSRF-protec.patch (text/plain), 4.57 KB, created by Jonathan Druart on 2024-09-16 13:39:25 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jonathan Druart

Created: 2024-09-16 13:39:25 UTC

Size: 4.57 KB

patch

obsolete

>From 9b107e57403b2200241ef971d221c4d17ab88b26 Mon Sep 17 00:00:00 2001
>From: Phil Ringnalda <phil@chetcolibrary.org>
>Date: Fri, 6 Sep 2024 14:33:17 -0700
>Subject: [PATCH] Bug 37786: members/cancel-charge.pl needs CSRF protection
>
>members/cancel-charge.pl will take either a POST or a GET, and as long as the
>accountline_id it is passed can be cancelled, will cancel it. That means any
>link you click anywhere while logged in to Koha might cancel a charge. It also
>takes a borrowernumber which isn't used for the cancelling, only to determine
>what account to show after a charge is cancelled, letting a malicious link
>show an account other than the one whose charge was just cancelled.
>
>Test plan:
> 1. Without the patch, Circulation - Checkout - search for the 'koha' patron
>    you log in as
> 2. Accounting - Create manual invoice - Make it a Manual fee of 100.00 and
>    Save
> 3. Pretending it's a well-disguised link in a spear-phishing email, load
>    http://localhost:8081/cgi-bin/koha/members/cancel-charge.pl?borrowernumber=5&accountlines_id=1
> 4. You are now looking at charges for the patron Acosta, Edna rather than for
>    the patron koha, but if you look at the patron koha, its 100.00 charge
>    has been cancelled.
> 5. Apply patch and reset_all (or if you don't, you'll have to manually adjust
>    the link to reflect the charge being accountlines_id 3 rather than 1)
> 6. Circulation - Checkout - search for the 'koha' patron you log in as
> 7. Accounting - Create manual invoice - Make it a Manual fee of 100.00 and
>    Save
> 8. Click the link http://localhost:8081/cgi-bin/koha/members/cancel-charge.pl?borrowernumber=5&accountlines_id=1
> 9. You got a 403 because you didn't pass the op cud-cancel, but if you did
>    pass that op, you would also get a 403 for having a cud- op in a GET (and
>    if you POST, you won't have a csrf_token)
>10. Checkout - search for koha - Accounting - Cancel charge
>11. Having done it the right way, you're now on koha's list of transactions,
>    where you can see you just cancelled it
>
>Sponsored-by: Chetco Community Public Library
>
>Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>---
> .../prog/en/modules/members/boraccount.tt     |  2 +-
> members/cancel-charge.pl                      | 30 +++++++++++--------
> 2 files changed, 19 insertions(+), 13 deletions(-)
>
>diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt
>index f8e5a90f1cf..cb2bc93ccd4 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt
>@@ -190,7 +190,7 @@
>         [% IF account.is_debit && account.amount == account.amountoutstanding && account.status != 'CANCELLED' && !(account.debit_type_code == 'PAYOUT') %]
>           <form method="post" action="/cgi-bin/koha/members/cancel-charge.pl">
>             [% INCLUDE 'csrf-token.inc' %]
>-            <input type="hidden" name="borrowernumber" value="[% patron.borrowernumber | html %]">
>+            <input type="hidden" name="op" value="cud-cancel">
>             <input type="hidden" name="accountlines_id" value="[% account.accountlines_id | html %]">
>             <button type="submit" class="btn btn-default btn-xs cancel-action">
>                 <i class="fa fa-ban"></i>
>diff --git a/members/cancel-charge.pl b/members/cancel-charge.pl
>index 5748fa54c46..0b6d756cd03 100755
>--- a/members/cancel-charge.pl
>+++ b/members/cancel-charge.pl
>@@ -32,15 +32,21 @@ my $flags = {
> my $type = 'intranet';
> my ($user, $cookie) = C4::Auth::checkauth($cgi, $authnotrequired, $flags, $type);
> 
>-my $borrowernumber = $cgi->param('borrowernumber');
>-my $accountlines_id = $cgi->param('accountlines_id');
>-
>-my $charge = Koha::Account::Lines->find($accountlines_id);
>-$charge->cancel(
>-    {
>-        branch   => C4::Context->userenv->{'branch'},
>-        staff_id => C4::Context->userenv->{'number'}
>-    }
>-);
>-
>-print $cgi->redirect('/cgi-bin/koha/members/boraccount.pl?borrowernumber=' . $borrowernumber);
>+my $op = $cgi->param('op') // q{};
>+
>+if ( $op eq "cud-cancel" ) {
>+    my $accountlines_id = $cgi->param('accountlines_id');
>+
>+    my $charge         = Koha::Account::Lines->find($accountlines_id);
>+    my $borrowernumber = $charge->patron->borrowernumber;
>+    $charge->cancel(
>+        {
>+            branch   => C4::Context->userenv->{'branch'},
>+            staff_id => C4::Context->userenv->{'number'}
>+        }
>+    );
>+    print $cgi->redirect( '/cgi-bin/koha/members/boraccount.pl?borrowernumber=' . $borrowernumber );
>+    exit;
>+}
>+
>+print $cgi->redirect('/cgi-bin/koha/errors/403.pl');
>-- 
>2.34.1

Actions: View | Diff | Splinter Review

Attachments on bug 37786: 171157 | 171543