Discovered because of the bug 27728 hit for koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt:191 - that posts without an op to members/cancel-charge.pl, because cancel-charge.pl doesn't have any ops, much less cud-ops. I don't have a full exploit, because that would require work, but Steps to reproduce: 1. Go to Checkout for the "koha" patron that you log in to ktd as 2. Accounting - Create manual invoice - Make it a Manual fee of 100.00 and Save 3. Pretend this is a well-disguised link in a spear-phishing email, and load http://localhost:8081/cgi-bin/koha/members/cancel-charge.pl?borrowernumber=5&accountlines_id=1 4. Wait, why am I looking at transactions for Acosta, Edna, who doesn't have any transactions? Not only does it wipe out charges from a GET with no token, it erases the only clue of what it just did by accepting a borrowernumber, not checking that it matches the borrowernumber for the accountlines_id, and showing transactions for the borrowernumber it was given, rather than the one for the accountline it cancelled. Seems like it would be at least a little difficult to discover the accountlines_id for a fee you owe, but no: go in to the library, ask them to print the charge, there's a nice Print button on the line for the charge, which includes "Fee ID: 1".
Fine, I'll take a swing at it, despite barely knowing Perl and not really knowing Koha. Feel free to just take it when you get tired of setting Failed QA.
Created attachment 171157 [details] [review] 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
Looks good to me at first glance, thanks!
Created attachment 171543 [details] [review] 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>
I get Error 403 in Step 3 of the test plan
In main? Looks like this landed silently last week in https://git.koha-community.org/Koha-community/Koha/commit/1f8b6d5daad407ed289a5b1f097e3ae57d6f452d
(In reply to Wainui Witika-Park from comment #5) > I get Error 403 in Step 3 of the test plan (In reply to Phil Ringnalda from comment #6) > In main? Looks like this landed silently last week in > https://git.koha-community.org/Koha-community/Koha/commit/ > 1f8b6d5daad407ed289a5b1f097e3ae57d6f452d Which version are you working on again, Wainui? Huh... looks like there's a bunch of pushing happening right now too?
(In reply to Phil Ringnalda from comment #1) > Fine, I'll take a swing at it, despite barely knowing Perl and not really > knowing Koha. Feel free to just take it when you get tired of setting Failed > QA. Mate, you've been doing awesome lately. You'll be a Koha pro in no time ;).
(In reply to David Cook from comment #7) > (In reply to Wainui Witika-Park from comment #5) > > I get Error 403 in Step 3 of the test plan > > (In reply to Phil Ringnalda from comment #6) > > In main? Looks like this landed silently last week in > > https://git.koha-community.org/Koha-community/Koha/commit/ > > 1f8b6d5daad407ed289a5b1f097e3ae57d6f452d > > Which version are you working on again, Wainui? > > Huh... looks like there's a bunch of pushing happening right now too? 23.05 :)
(In reply to Phil Ringnalda from comment #6) > In main? Looks like this landed silently last week in > https://git.koha-community.org/Koha-community/Koha/commit/ > 1f8b6d5daad407ed289a5b1f097e3ae57d6f452d Katrin, was it expected?
Do I need to apply this to 23.05.x-security? I get Error 403 in Step 3 of the test plan
members/cancel-charge.pl has CSRF protection (the old way) in 23.05.x and 23.11.x so don't think this patch is relevant there. Looks like the old CSRF protection was removed in 24.05.x by Bug 34478 so will add that dependency to this report