Summary: | members/cancel-charge.pl needs CSRF protection | ||
---|---|---|---|
Product: | Koha | Reporter: | Phil Ringnalda <phil> |
Component: | Patrons | Assignee: | Phil Ringnalda <phil> |
Status: | CLOSED FIXED | QA Contact: | Jonathan Druart <jonathan.druart> |
Severity: | major | ||
Priority: | P3 | CC: | aleisha, dcook, didier.gautheron, fridolin.somers, gmcharlt, jonathan.druart, katrin.fischer, kyle.m.hall, laurent.ducos, lisette, lucas, m.de.rooy, martin.renvoize, nick, philippe.blouin, tomascohen, wainuiwitikapark |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: |
24.11.00,24.05.05
|
Circulation function: | |
Bug Depends on: | 34478 | ||
Bug Blocks: | 37728 | ||
Attachments: |
Bug 37786: members/cancel-charge.pl needs CSRF protection
Bug 37786: members/cancel-charge.pl needs CSRF protection |
Description
Phil Ringnalda
2024-08-30 05:36:34 UTC
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 |