|
Description
Jonathan Druart
2023-08-04 06:59:29 UTC
(In reply to Jonathan Druart from comment #0) > - Convert all GET to POST (154 occurrences) Not all of them. We have GET forms that do not modify state. Then maybe we need a way to tell that those forms are expected GETs. Looks good to me. We need a plan and coordination to prevent duplication of effort. And we need commitment to do this in reasonable time. Surely we will have rebasing issues and backporting trouble along the way ;) (In reply to Marcel de Rooy from comment #2) > Looks good to me. We need a plan and coordination to prevent duplication of > effort. And we need commitment to do this in reasonable time. > Surely we will have rebasing issues and backporting trouble along the way ;) I am working on a POC here. Created attachment 154241 [details] [review] Bug 34478: Script to catch GET forms and its output that have been modified to show the state of the progres (search for "TO BE CONTINUED") "stateless" => "get" can still be used for this form "modified" => "get" replaced with "post" and there is an "op" parameter "???" or "TODO" => needs more attention Created attachment 154242 [details] [review] Bug 34478: Replace get with post when needed This is what has been marked as done in "csrf_get.txt" Created attachment 154243 [details] [review] Bug 34478: Replace csrf_token input with include file perl -p -i -n -e 's#<input type="hidden" name="csrf_token" value="\[% csrf_token \| html %]" />#[% INCLUDE 'csrf-token.inc' %]#g' **/*.tt **/*.inc This should have actually been done at the same time as "Bug 30524: (QA follow-up) Only generate CSRF token if it will be used" Created attachment 154244 [details] [review] Bug 34478: Replace csrf_token input with include file - manual A couple of left not caught by the previous regex Still TODO: % git grep csrf_token **/*.inc **/*.tt still shows example that needs to be replaced, later (because we use GET) Created attachment 154245 [details] [review] Bug 34478: Remove generate_csrf from pl We do not longer need to generate_csrf from pl files TODO - members/boraccount.tt and sco/sco-main.tt needs to be adjusted Created attachment 154246 [details] [review] Bug 34478: Add a script to add missing csrf to post forms - csrf_add_missing_csrf Created attachment 154247 [details] [review] Bug 34478: Add missing CSRF token to POST forms This is the result of % perl csrf_add_missing_csrf.pl Created attachment 154248 [details] [review] Bug 34478: Add a script to catch missing 'op' in POST forms - catch_missing_op Created attachment 154249 [details] [review] Bug 34478: Output of catch_missing_op.pl % perl catch_missing_op.pl > missing_op.txt Created attachment 154250 [details] [review] Bug 34478: Add 'op' to opac-user.tt Created attachment 154251 [details] [review] Bug 34478: Add 'op' to opac-passwd Created attachment 154252 [details] [review] Bug 34478: Adjust missing_op.txt To show what we have done in the previous "Add 'op' to " commits Created attachment 154253 [details] [review] Bug 34478: Check CSRF in get_template_and_user Not sure this is the right place in get_template_and_user Will have to test login and 2FA Created attachment 154254 [details] [review] Bug 34478: Remove check_csrf from pl files We should no longer need to check CSRF token from pl files TODO - there is a change for some files where we returned 403 This is what I have in mind for 1. What do you think? That's still a lot of work, but I think it will require less code in controllers and will make future occurrences harder to be introduced. And at least there is kind of a plan now? I won't work more on it until some of you agree on the concept and are willing to help. I don't think "Check CSRF in get_template_and_user" is running in the right place. I think that check will only run if checkauth fails. But that check won't work anyway because there are times when we have a GET with an "op" that isn't state changing. For example: http://localhost:8081/cgi-bin/koha/tools/additional-contents.pl?op=add_form&id=3&editmode=wysiwyg I was looking again at the number of scripts we have and it's less than I was thinking (once you exclude installer/, opac/, misc/, and value_builder/):
find -name "*.pl" | egrep -v "(/installer/|/opac/|/misc/|/value_builder)" | wc -l
405
--
Interestingly, a lot of scripts are actually a lot shorter than you'd think.
find -name "*.pl" -exec wc -l {} + | egrep -v "(/installer/|/opac/|/misc/|/value_builder)" | sort -n
That could actually give us some direction on what scripts need the most testing (ie show us the ones with the most complexity).
(In reply to Jonathan Druart from comment #0) > Suggestions: > 1. > IMO we should not add the csrf_token.inc for all <form> AND the check in the > controllers (pl). That will be a nightmare to write, test, and backport! > However we could easily add the include when missing to all forms (we have > the test from bug 34426 that can be used as a base). > Then we should: > - Convert all GET to POST (154 occurrences) > - Make sure all form has the same 'op' parameter > - In a low level sub (get_template_and_user?) we could check for: Any > requests with the parameter 'op' must be a POST with a valid CSRF token > => We spend time but at least we clean things! > => We can do that in different steps and backport them without disclosing > the CSRF flaws. Generally speaking, I agree that we should convert many GETs to POSTs. (I mean in terms of "forms" as well as URLs. There are way too many state changes happening through links, which are easily exploitable with CSRF.) We should do the CSRF token check for *all* POSTs. That would make it so that future developments won't forget the CSRF token for their POSTs. The only issue I see is enforcing POSTs server-side. As I noted earlier, many requests with an 'op' parameter are currently GET requests. I suppose we could change them to POST requests, although that would mean they would no longer bookmarkable/shareable. That would be annoying. There are many legitimate times where we should have GET requests I think. -- One option could be to default to any request with an 'op' parameter must be a POST, and then we have a layer that checks for exceptions. (In reply to David Cook from comment #21) > One option could be to default to any request with an 'op' parameter must be > a POST, and then we have a layer that checks for exceptions. I'm still a bit skeptical about how consistently we've used the 'op' parameter though. An example of a long script without it is ./circ/returns.pl ./reports/guided_reports.pl has an 'op' parameter but it's almost useless. It uses 'phase' for the most part. -- I keep thinking that we will need to touch each controller script. But I keep wondering how we can enforce state changes being done by POST rather than by GET, so that we don't re-introduce vulnerabilities down the road. Even if we change a "form" or don't use a link, the controller scripts are vulnerable so long as we're not checking the "request_method". I think we need to start incorporating that in our scripts. I do want to highlight that so long as we add CSRF protection to high impact endpoints like password changes, system preferences, additional contents, borrower add/edit, etc. We can do lower impact endpoints later. Having someone take over your account is much worse than having them return your book for you. Good work, Jonathan.
Most crucial is the Auth change. We should really check that one. But if we add it somewhere there, we can remove a lot of cruft.
Are we posting without op somehwere? Apart from svc or API? (See below too.) Do we forget other categories?
The op approach is quite pragmatic and feels a bit awkward. But could help us now to make progress. Can we remove it later?
How much work would be left in checking the remaining templates?
We have enough time for master, but how do we envision backporting? Should we, like David suggests, make a short list of the ones that we really want backported? With an adjusted auth change?
This one has probably no op btw:
<form action="/cgi-bin/koha/svc/barcode" method="post" id="barcodegen">
+ [% INCLUDE 'csrf-token.inc' %]
(In reply to David Cook from comment #22) > I keep thinking that we will need to touch each controller script. > > Even if we change a "form" or don't use a link, the controller scripts are > vulnerable so long as we're not checking the "request_method". I think we > need to start incorporating that in our scripts. And it's fewer scripts than we might think. find -name "*.pl" -exec wc -l {} + | egrep -v "(/installer/|/opac/|/misc/|/value_builder|/t|/errors|/xt)" | sort -n | wc -l 345 find opac -name "*.pl" -exec wc -l {} + | sort -n | wc -l 94 The vast majority of these scripts are actually very small and many of them won't actually need a change at all like ./reports/reports-home.pl So let's not let ourselves be overwhelmed by our task. Let's fix the highest risk controllers and then go from there. Do we move forward with this approach or do we abandon it? (In reply to Marcel de Rooy from comment #24) > Are we posting without op somehwere? Apart from svc or API? (See below too.) If we don't, we will add it. > Do we forget other categories? Maybe. > The op approach is quite pragmatic and feels a bit awkward. But could help > us now to make progress. Can we remove it later? I don't think so. > How much work would be left in checking the remaining templates? I don't know, we need to agree on a plan then go deeper. > We have enough time for master, but how do we envision backporting? Should > we, like David suggests, make a short list of the ones that we really want > backported? With an adjusted auth change? We should not backport this one. We should fix the major ones and backport them. Then wait for a major release to embed this. Created attachment 155971 [details] [review] Bug 34478: Script to catch GET forms and its output that have been modified to show the state of the progres (search for "TO BE CONTINUED") "stateless" => "get" can still be used for this form "modified" => "get" replaced with "post" and there is an "op" parameter "???" or "TODO" => needs more attention Created attachment 155972 [details] [review] Bug 34478: Replace get with post when needed This is what has been marked as done in "csrf_get.txt" Created attachment 155973 [details] [review] Bug 34478: Replace csrf_token input with include file perl -p -i -n -e 's#<input type="hidden" name="csrf_token" value="\[% csrf_token \| html %]" />#[% INCLUDE '\''csrf-token.inc'\'' %]#g' **/*.tt **/*.inc This should have actually been done at the same time as "Bug 30524: (QA follow-up) Only generate CSRF token if it will be used" Created attachment 155974 [details] [review] Bug 34478: Replace csrf_token input with include file - manual A couple of left not caught by the previous regex Still TODO: % git grep csrf_token **/*.inc **/*.tt still shows example that needs to be replaced, later (because we use GET) Created attachment 155975 [details] [review] Bug 34478: Remove generate_csrf from pl We do not longer need to generate_csrf from pl files TODO - members/boraccount.tt and sco/sco-main.tt needs to be adjusted Created attachment 155976 [details] [review] Bug 34478: Add a script to add missing csrf to post forms - csrf_add_missing_csrf Created attachment 155977 [details] [review] Bug 34478: Add missing CSRF token to POST forms This is the result of % perl csrf_add_missing_csrf.pl Created attachment 155978 [details] [review] Bug 34478: Add a script to catch missing 'op' in POST forms - catch_missing_op Created attachment 155979 [details] [review] Bug 34478: Output of catch_missing_op.pl % perl catch_missing_op.pl > missing_op.txt Created attachment 155980 [details] [review] Bug 34478: Add 'op' to opac-user.tt Created attachment 155981 [details] [review] Bug 34478: Add 'op' to opac-passwd Created attachment 155982 [details] [review] Bug 34478: Adjust missing_op.txt To show what we have done in the previous "Add 'op' to " commits Created attachment 155983 [details] [review] Bug 34478: Check CSRF in get_template_and_user Not sure this is the right place in get_template_and_user Will have to test login and 2FA Created attachment 155984 [details] [review] Bug 34478: Remove check_csrf from pl files We should no longer need to check CSRF token from pl files TODO - there is a change for some files where we returned 403 Created attachment 155985 [details] [review] Bug 34478: Move C4::Auth check Created attachment 155986 [details] [review] Bug 34478: op-cud - Adjust C4::Auth code Created attachment 155987 [details] [review] Bug 34478: op-cud - Add a script to rename op with op-cud in templates Created attachment 155988 [details] [review] Bug 34478: op-cud - Rename op with op-cud in templates This is the result of perl rename_op_with_op-cud.pl Created attachment 155989 [details] [review] Bug 34478: op-cud - Trick CGI directly The previous implementation had a flaw. It was wrong to force a POST when 'op' parameter was there (eg. op="list"). In this 'op-cud' version I am suggestion the same thing, but with a op-cud parameter (cud is crud without 'r'). op-cud won't be returned if the request is a GET op won't be returned if the request is not a GET op will be set to op-cud so that we don't have a adjust the controllers. C4::Auth::checkauth will check csrf if op-cud exists. Wait a bit more here. Created attachment 155990 [details] [review] Bug 34478: op-cud - Trick CGI directly Ideally we need bug 34862 to be pushed before this. Well, I am missing the obvious here... We can GET with op and without CSRF token, and we pass through everything (lol!) I think I have been trying to avoid the unavoidable... editing the controllers one by one, manually. Maybe... maybe... to continue with this idea, we should have "cud" operations prefixed with "cud-" (cud-add, cud-delete, etc.). The mess is the lack of consistency (no, really?) in the different $op's values (add, delete, add_form, add_validate, delete, delete_confirm, deleted_confirmed, etc.). I don't see how we could modify them programmatically... (In reply to Jonathan Druart from comment #51) > I think I have been trying to avoid the unavoidable... editing the > controllers one by one, manually. > > Maybe... maybe... to continue with this idea, we should have "cud" > operations prefixed with "cud-" (cud-add, cud-delete, etc.). The mess is the > lack of consistency (no, really?) in the different $op's values (add, > delete, add_form, add_validate, delete, delete_confirm, deleted_confirmed, > etc.). > > I don't see how we could modify them programmatically... I agree. I think that we're going to need to do manual work here. I'm still thinking but here's what my current thoughts are: 1. Temporarily add an "enforce_csrf" parameter to "get_template_and_user" which requires all POST requests to have a valid CSRF token (this allows controller support be gradually rolled out - once we're satisfied coverage is complete this will be deprecated and enforcement will no longer be optional) (enforcing CSRF for all POST requests provides future proofing to make it harder for people to re-introduce CSRF vulnerabilities) 2. HTML coding guideline that state changes by form elements must be done by POST and not by GET, and include a CSRF token 3. Javascript coding guideline that state changes in AJAX requests must be done by POST and not by GET, and include a CSRF token 4. Perl coding guideline that state changes must be validated by checking the "request_method" is POST 5. A csrf_token will be stored in a "meta" tag in the "head" of the HTML, so that Javascript can easily access it for AJAX requests. (I've seen this done in a number of anti-CSRF middlewares.) 6. Add a new "output_and_exit" with an "invalid_method" error code. (Note that this doesn't yet include the Mojolicious REST API. For external REST API consumers, we'll need a way to provide them with CSRF tokens. I'll think more about this use case.) -- Looking at ./members/member-password.pl we'd change the current CSRF check into a POST check, and add the "enforce_csrf" into "get_template_and_user". We'll need to add a CSRF token to the login form, since that can POST to ./members/member-password.pl as well. SSO should be fine since redirecting from an IdP would be a GET. Previously we have added CSRF tokens to GET requests that do state changes, but this isn't a best practice, and it's something that we'll want to avoid going forward. I think that's implied with what I've said above, but reiterating it anyway. I know there are a lot of state changes in Koha that happen with GET requests sent via links, but I do have a method for fairly easily refactoring those. Bug 34650 is an example where the "a" element link is still clicked but it triggers Javascript which submits a hidden form. Owen might also have some thoughts on styling that can be used for replacement buttons to take over from links. I'm looking through a list of staff interface scripts to see what we need to update. I've reviewed about 50 out of 345. So far I've found 6 "yes" and 4 "maybe". It's actually quite interesting looking at scripts I've never looked at before. A remarkable number fill an API role and should just be replaced. There's also some really old terrible coding patterns like ./reports/manager.pl which loads ./reports/itemtypes.plugin, but those are unrelated to CSRF. They're just... yikes. ./admin/import_export_framework.pl is already separating the GET and POST handling (although it doesn't have any error handling). (In reply to David Cook from comment #55) > ./admin/import_export_framework.pl is already separating the GET and POST > handling (although it doesn't have any error handling). But it only uses check_cookie_auth() and does not use get_template_and_user(). So that needs to be taken into account... checkauth() runs in get_template_and_user() and it calls check_cookie_auth() but not in all situations. That suggests to me that we might need a function like "validate_csrf_token({ controller => $cgi })" which can be run in "get_template_and_user" but has to be manually added to scripts without "get_template_and_user" which just use "check_cookie_auth" or "checkauth" manually... -- Because we use CGI scripts instead of a central controller it's hard for us to take actions that affect all pages. Out of 124 staff interface scripts checked, 43 will need to be updated which is roughly 1/3. These scripts are also all less than 100 lines long so far, so they should be very easy to update. (And if we coordinate our SO/QA we can hopefully power through nicely.) I have some other work to do now, but I'll get back to this next week. Once I've done my review, I'll share the results. Maybe we can do a little framapad or Google Doc and take chunks of the list. -- In some ways, it's too bad that we don't have Bug 31380 pushed, because a lot of these small scripts would be so easy to port over. But anyway... (In reply to David Cook from comment #56) > (In reply to David Cook from comment #55) > > ./admin/import_export_framework.pl is already separating the GET and POST > > handling (although it doesn't have any error handling). > > But it only uses check_cookie_auth() and does not use > get_template_and_user(). So that needs to be taken into account... > > checkauth() runs in get_template_and_user() and it calls check_cookie_auth() > but not in all situations. > > That suggests to me that we might need a function like > "validate_csrf_token({ controller => $cgi })" which can be run in > "get_template_and_user" but has to be manually added to scripts without > "get_template_and_user" which just use "check_cookie_auth" or "checkauth" > manually... Did you read my patches from yesterday? We need some more coordination here. Otherwise we are spending valuable time without getting much further. (In reply to Marcel de Rooy from comment #59) > We need some more coordination here. Otherwise we are spending valuable time > without getting much further. I agree about needing coordination, but I do think that we're still in the brainstorming step. I'm doing anti-CSRF work on a different Perl project, so I'm hoping to bring back some lessons to this work in the near future. Created attachment 156070 [details] [review] Bug 34478: op =~ ^cud- Created attachment 156071 [details] [review] Bug 34478: [DO NOT PUSH] Cypress tests - yarn add cypress-mysql Created attachment 156072 [details] [review] Bug 34478: [DO NOT PUSH] Cypress tests - cypress-mysql config Created attachment 156073 [details] [review] Bug 34478: [DO NOT PUSH] Cypress tests This should not be pushed, we are not ready on jenkins. Ok, certainly the last iteration for me here. Before starting here you need to read comment 0 and apply the patches and read the commit messages, to understand the thinking path. A bit of detail however: * With bug 34862 we remove blocking-errors.inc * csrf_get.pl generates csrf_get.txt => a list of GET forms (to replace?) That should be put on a collaborative pad * Use csrf-token.inc when needed * Remove generate_csrf from the pl, we will do it later in C4::Auth * Add csrf_token.inc to all POST forms * Catch missing 'op' from POST forms * There is a try with "op-cud", but it was not working, however the main idea is kept in the next commits * op =~ ^cud- => Last suggestion, we have only one 'op', but we prefixed the value with 'cud-' to flag when the operation will not be stateless. It must not be done with a GET * cypress tests My suggestion for now would be (for pl/tt, so the first point from comment 0): 1. Identify and fix the critical forms and backport them. Don't backport anything else. 2. Replace GET forms with POST when needed 3. Replace POST forms with GET when needed (ie. on this last example, delete_confirm does not need to be POSTed) 4. Rename $action (and other "wrongly" named variables) with $op 5. Adjust the "cud" $op's values (ie. prefix with 'cud-', add=>cud-add, etc.) 6. Add 'op' where missing (see missing_op.txt, must be on a pad as well) 7. Continue with what we have here 8. Investigate svc, REST API (bug 34451), and ajax requests from .js files (2, 3, 4 from comment 0) Feel free to suggest something better than 'cud'. But it seems to exist https://en.wiktionary.org/wiki/CUD and makes sense to me. It needs to be short and meaningful. The most important code is in C4::Auth (search for 'cud-') and debian/templates/plack.psgi. At this point the only place where the /^cud-/ suggestion is implement is /admin/branches.pl, no need to test anything else! Who agrees on that? Who has time to contribute/help? Which timeline? To run the cypress tests you need to: % yarn install % docker run -it \ -v $PWD:/e2e \ -v /tmp/.X11-unix:/tmp/.X11-unix \ -w /e2e \ -e DISPLAY \ --entrypoint cypress \ --network koha_kohanet \ cypress/included:12.14.0 open --project . Or make your host resolves 'db', or (if you are not using the UI) maybe simply run them within ktd, I have not tried. Created attachment 156074 [details] [review] Bug 34478: [DO NOT PUSH] Cypress tests This should not be pushed, we are not ready on jenkins. (In reply to Jonathan Druart from comment #65) > My suggestion for now would be (for pl/tt, so the first point from comment > 0): > 1. Identify and fix the critical forms and backport them. Don't backport > anything else. > 2. Replace GET forms with POST when needed > 3. Replace POST forms with GET when needed (ie. on this last example, > delete_confirm does not need to be POSTed) > 4. Rename $action (and other "wrongly" named variables) with $op > 5. Adjust the "cud" $op's values (ie. prefix with 'cud-', add=>cud-add, etc.) > 6. Add 'op' where missing (see missing_op.txt, must be on a pad as well) > 7. Continue with what we have here > 8. Investigate svc, REST API (bug 34451), and ajax requests from .js files > (2, 3, 4 from comment 0) > Who agrees on that? > Who has time to contribute/help? > Which timeline? I don't understand the example from #3 but otherwise I think I agree for the most part. (For the Javascript-based requests, I think the way to do it is to have a <meta> element on every page with the CSRF token which the Javascript can fetch and use. The tricky part for the REST API will be existing external consumers of the API outside of Koha, especially for HTTP Basic Auth users.) I'll make time to contribute/help. In terms of timeline... I think that we could actually push the op =~ ^cud- check any time as that won't have any effect on existing code until we start changing controllers. That would also allow us to add a new coding guideline. Then we can incrementally update controllers at a pace that we can manage. (In reply to David Cook from comment #68) > (In reply to Jonathan Druart from comment #65) > > 2. Replace GET forms with POST when needed > > 3. Replace POST forms with GET when needed (ie. on this last example, > > delete_confirm does not need to be POSTed) > I don't understand the example from #3 but otherwise I think I agree for the > most part. See the "Bug 34478: op =~ ^cud-". For "delete_confirm" (the confirmation screen for the deletion), I don't think we should POST it, and do not need to add a CSRF token to it. However we will want it for the "Yes, delete" button (deleted_confirmed). Without feedback and help I will abandon this one in the next couple of weeks. (In reply to Jonathan Druart from comment #70) > Without feedback and help I will abandon this one in the next couple of > weeks. Agreed. We need to move on this. I think it's time to move forward. I'd like to sign-off but it's not currently applying. I got the "sha1 information is lacking or useless" error on the "Bug 34478: Add missing CSRF token to POST forms". Can you rebase? I'm back from my holidays now, so I can take a look at this too. -- On a side note, I think that this will probably break some Koha plugins, but they'll just need to update. (In reply to Kyle M Hall from comment #71) > (In reply to Jonathan Druart from comment #70) > > Without feedback and help I will abandon this one in the next couple of > > weeks. > > Agreed. We need to move on this. I think it's time to move forward. I'd like > to sign-off but it's not currently applying. I got the "sha1 information is > lacking or useless" error on the "Bug 34478: Add missing CSRF token to POST > forms". Can you rebase? It was not ready for testing. See comment 65. You can apply the patches on top of 266194321453d096a1fd1a9f80195dfb77ad5258 + 34426 + 34862 Just sharing some of my CSRF adventures on other projects. I've been adding anti-CSRF to a small Catalyst app. For HTML forms, fortunately, there is a lot of template re-use on that project, so I only had to add CSRF tokens to maybe 20 templates to cover the entire app. For AJAX calls to the API, I fetch a "csrf-token" from a meta tag that I write out in my template wrapper. (See "Client Side CSRF" on the OWASP anti-CSRF cheatsheet for more details.) Server-side in the Perl I enforce CSRF token requirements by using "auto_check_csrf_token" in the Catalyst::Plugin::CSRFToken app, which requires it for all POST, PUT, and PATCH requests. I've noticed that Content-Security-Policy violation reports will get blocked by the above, so I had to add an exception writing a custom handler supported by Catalyst::Plugin::CSRFToken. -- I'm working on a solution for external API calls to the application. Like DSpace, I'll probably send the CSRF token in a previous GET request, and then the client calling the API will need to provide that CSRF token. This does add complexity for using the API. But I think it's something that we need to do. It's now or never for me. (In reply to Jonathan Druart from comment #75) > It's now or never for me. We're going into summer holidays in Australia soon, so in 2 weeks I'll have little availability until February. But we're at a good point in the release cycle, and it sounds like Kyle is on board too. Let's poke Martin too as I know he want anti-CSRF too. Are patches ready for testing or need a rebase? Let's get this one done. I am supporting this too. Just have been waiting for resulting patches from the brainstorm session :) (In reply to David Cook from comment #76) > Are patches ready for testing or need a rebase? (In reply to Marcel de Rooy from comment #77) > I am supporting this too. Just have been waiting for resulting patches from > the brainstorm session :) Read comment #65. Please confirm that you understand what I am suggesting and that you agree with the suggestion. Then that you are ready to provide a fast SO or QA stamp when a patch will be attached. Then I will write the full patch. (In reply to Jonathan Druart from comment #78) > (In reply to David Cook from comment #76) > > Are patches ready for testing or need a rebase? > > (In reply to Marcel de Rooy from comment #77) > > I am supporting this too. Just have been waiting for resulting patches from > > the brainstorm session :) > > Read comment #65. > Please confirm that you understand what I am suggesting and that you agree > with the suggestion. Then that you are ready to provide a fast SO or QA > stamp when a patch will be attached. > > Then I will write the full patch. I think that I understand what you're suggesting, and I agree. I have time over the next 2 weeks. My availability in January is quite limited with most time being available at the end of the month. So I might be better for a fast SO than a fast QA. I have pushed a remote branch to our security repo (rebased and with new commits). https://git.koha-community.org/Koha-community/security/src/branch/bug_34478 (If you need access let me know) There are still a lot of things to do here, and not the most interesting ones. I think we should now focus on the "csrf_get.txt" file. It contains the different GET forms, and some of them need to be replaced with POST. Then we will have to focus on the POST that needs to be GET (see the "Manual fix" commits). I won't do all of this alone. I have done my part already. Now I need at least 2 other people to help with the remaining steps. There is also a commit for debugging purpose: Bug 34478: Display programming errors in case plack.psgi caught something suspicious It will display an alert on the UI if something is wrong with the 'op' parameter. Blimey, there's loads of commits there! Thanks for all the hard work to date Jonathan! I'll try and take a look at this over xmas to get to grips with whats left and how I can help. Next step should be to continue the list on this pad (convert GET to POST): https://annuel.framapad.org/p/koha_34478_get_forms (In reply to Jonathan Druart from comment #83) > Next step should be to continue the list on this pad (convert GET to POST): > > https://annuel.framapad.org/p/koha_34478_get_forms Next step before sign off or next step in the process overall? (In reply to David Cook from comment #84) > (In reply to Jonathan Druart from comment #83) > > Next step should be to continue the list on this pad (convert GET to POST): > > > > https://annuel.framapad.org/p/koha_34478_get_forms > > Next step before sign off or next step in the process overall? Not really for sign off. (In reply to Jonathan Druart from comment #65) > My suggestion for now would be (for pl/tt, so the first point from comment > 0): > 1. Identify and fix the critical forms and backport them. Don't backport > anything else. => Not really done, but not necessary for this bug. > 2. Replace GET forms with POST when needed The pad we are talking about. > 3. Replace POST forms with GET when needed (ie. on this last example, > delete_confirm does not need to be POSTed) Not done. > 4. Rename $action (and other "wrongly" named variables) with $op Not done. > 5. Adjust the "cud" $op's values (ie. prefix with 'cud-', add=>cud-add, etc.) Should have been done (scripted). > 6. Add 'op' where missing (see missing_op.txt, must be on a pad as well) Not done. > 7. Continue with what we have here > 8. Investigate svc, REST API (bug 34451), and ajax requests from .js files > (2, 3, 4 from comment 0) Not done. We need someone else to lead this one, I am failing at communicating correctly it seems. (In reply to Jonathan Druart from comment #85) > We need someone else to lead this one, I am failing at communicating > correctly it seems. I think that you're doing great. I just want to know how myself and others can help you out :). I was thinking about spinning through https://annuel.framapad.org/p/koha_34478_get_forms but I was just wondering if that was something that we needed now or later. (On the other Perl app I added anti-CSRF to, I've added tokens to the POST forms in the templates, and I'm requiring tokens for all PUT/PATCH/POST requests (including API requests), but I haven't yet reviewed all my GETs to check which need to be converted to POSTs. It's my last step.) Update: GET forms from the pad has been updated and some commits have been pushed. https://annuel.framapad.org/p/koha_34478_get_forms Now we need to focus on form without "method" parameter. The list has been generated using csrf_no_action.pl and the output is on the following pad: https://annuel.framapad.org/p/koha_34478_no_action_forms Note that 'method="get"' has been added to these forms (commit "Add method="get" to forms without method") to make it explicit. Be careful with the commit "Bug 34478: op =~ ^cud- in pl/pm". It certainly generated some incorrect changes! (In reply to Jonathan Druart from comment #88) > Be careful with the commit "Bug 34478: op =~ ^cud- in pl/pm". > > It certainly generated some incorrect changes! Some changes to show=>cud-show and select=>cud-select (at least) are suspicious. 4. Rename $action (and other "wrongly" named variables) with $op See https://annuel.framapad.org/p/koha_34478_rename_with_op I've created a new pad for the "remaining" work (those which does not fit in the previous cases, or the hard ones we want to keep for later) https://annuel.framapad.org/p/koha_34478_remaining Remote branches rebase against master. Already a lot of conflicts and new forms added! We really need to speed up! Remote branche rebased against master. Already a lot of conflicts and new forms added! We really need to speed up! (In reply to Jonathan Druart from comment #65) > 5. Adjust the "cud" $op's values (ie. prefix with 'cud-', add=>cud-add, etc.) Nothing to do here, we have been doing it in other tasks. > 6. Add 'op' where missing (see missing_op.txt, must be on a pad as well) See *NEW* pad: https://annuel.framapad.org/p/koha_34478_missing_op Work is actively in progress by some of us. Please join if you are willing to help. IMO this must be high priority for everybody and we should block everything until it's pushed. At least nothing trivial/minor/cosmetic, or even big, should be pushed to master. This is a nightmare to fix and even worst to rebase. (In reply to Jonathan Druart from comment #95) > IMO this must be high priority for everybody and we should block everything > until it's pushed. > > At least nothing trivial/minor/cosmetic, or even big, should be pushed to > master. > > This is a nightmare to fix and even worst to rebase. I agree but what does Katrin think as release manager? I don't think I've seen her comment on these Bugzilla issues. As a security issue, it couldn't be placed on the road map, but I think we'll need her support in order to really tackle this one. (In reply to David Cook from comment #96) > I agree but what does Katrin think as release manager? I don't think I've > seen her comment on these Bugzilla issues. > > As a security issue, it couldn't be placed on the road map, but I think > we'll need her support in order to really tackle this one. I just saw Katrin's email saying "Temporary half on pushing patches". That's very exciting!!! -- I'll take a look at https://annuel.framapad.org/p/koha_34478_missing_op later today. It looks like I don't have push privileges. Can I get those? Trying to make sense of all the pads. I started on https://annuel.framapad.org/p/koha_34478_missing_op and that's led me to a few breakages that I've looked at fixing. (In reply to David Cook from comment #98) > It looks like I don't have push privileges. Can I get those? Try again now. We have frozen master/main, so I think you could count that as full support from the RM. We need all the resources we have focused on this one right now. We are doing in C4/Auth.pm here:
C4::Output::output_and_exit( $in->{query}, $cookie, $template, 'wrong_csrf_token' );
output_and_exit uses template variable blocking_error to pass it down.
But many templates do not have the blocking_errors.inc and do not contain a blocking_error variable. So this does result in empty forms, confusion, etc?
When testing changes, I did not yet see the error on my screen. Did you?
This one is important: We need some additional changes in C4/Auth (incl get_template_and_user) for the login process. Check the anonymous session token and the CSRF token. I skipped adding some cud- $op for auth.tt forms, since it felt awkward. But now the test looks at cud- and we do not check. Any thoughts? Probably we have similar issue for svc or REST API ? (In reply to Marcel de Rooy from comment #103) > Probably we have similar issue for svc or REST API ? All svc scripts that aren't meant for external use should be CSRFed. Any svc scripts that are for external use should be replaced with REST API endpoints. Based on my research and understanding, the REST API should require a CSRF token *only* when using cookie based authentication, but not when using token based authentication ( basic auth is a gray area where we could possibly require CSRF but that would break it's ability to be used outside Koha, which is the only time we use basic auth, so really it should left as is with a stern warning ;) I think this is big work that is out of scope for the current project ( which is already very big ), but would be a good followup. (In reply to Jonathan Druart from comment #99) > (In reply to David Cook from comment #98) > > It looks like I don't have push privileges. Can I get those? > > Try again now. Legend! Thanks, Jonathan. I just pushed some changes for "Administration > Z39.50/SRU servers". I think I did the changes correctly, but let me know if there's any issue with them. I removed "cud-" from some GET view ops, converted the delete GET to POST (using a Javascript-driven submit method I've used before, but not 100% sure if that's the method we all want to use or not), and added cud-search for a search POST (although perhaps I should've converted it to a GET instead?) (In reply to Kyle M Hall from comment #104) > I think this is big work that is out of scope for the current project ( > which is already very big ), but would be a good followup. Agreed. I think we focus on the old school HTML web app, and later we can focus on the REST API. Divide and conquer... (In reply to Marcel de Rooy from comment #102) > This one is important: > > We need some additional changes in C4/Auth (incl get_template_and_user) for > the login process. Check the anonymous session token and the CSRF token. > I skipped adding some cud- $op for auth.tt forms, since it felt awkward. But > now the test looks at cud- and we do not check. > > Any thoughts? I noticed for auth.tt and opac-auth.tt that the CSRF token wasn't being generated for anonymous users, because the sessionID wasn't being passed to the template. I've fixed that. Same goes for anonymous users and /cgi-bin/koha/installer/install.pl. Fixing that one too. Note the fix is just for CSRF token generation. Not the anti-CSRF work itself. (In reply to Marcel de Rooy from comment #102) > This one is important: > > We need some additional changes in C4/Auth (incl get_template_and_user) for > the login process. Check the anonymous session token and the CSRF token. > I skipped adding some cud- $op for auth.tt forms, since it felt awkward. But > now the test looks at cud- and we do not check. > > Any thoughts? I added a "cud-login" for auth.tt and then removed the CSRF token, and it didn't fail, which was surprising. It looks like we've got "skip_csrf_check" to always true, so at the moment it's impossible to get a CSRF error... and if you disable it then it fails regardless because of a CSRF_INVALID_SIGNATURE. I suppose because the CSRF token was for the anonymous session and we've changed to an authenticated session by the time we're checking. I think that we're doing our CSRF checking too late. It needs to be one of the first things done in the processing of a request. Even if we're not failing at that time, we need to do the check at that time. (In reply to David Cook from comment #107) > I noticed for auth.tt and opac-auth.tt that the CSRF token wasn't being > generated for anonymous users, because the sessionID wasn't being passed to > the template. I've fixed that. > > Same goes for anonymous users and /cgi-bin/koha/installer/install.pl. Fixing > that one too. Note that I've reverted these for now since they can't work without the changes I mention in the above comment... (In reply to Marcel de Rooy from comment #101) > We are doing in C4/Auth.pm here: > C4::Output::output_and_exit( $in->{query}, $cookie, $template, > 'wrong_csrf_token' ); > output_and_exit uses template variable blocking_error to pass it down. > > But many templates do not have the blocking_errors.inc and do not contain a > blocking_error variable. So this does result in empty forms, confusion, etc? > > When testing changes, I did not yet see the error on my screen. Did you? With bug 34862 it should be everywhere now (unless exotic places like modals). Please keep track of them. (In reply to Marcel de Rooy from comment #102) > This one is important: > > We need some additional changes in C4/Auth (incl get_template_and_user) for > the login process. Check the anonymous session token and the CSRF token. > I skipped adding some cud- $op for auth.tt forms, since it felt awkward. But > now the test looks at cud- and we do not check. > > Any thoughts? I am not sure I understand: if there is no "op" parameter you should not be caught by the test. BUT I think we should actually enforce this condition (later or now): if it's a POST then we must have a "op" parameter. In this case we will need to discuss it the "login" case a bit more. (In reply to Kyle M Hall from comment #104) > (In reply to Marcel de Rooy from comment #103) > > Probably we have similar issue for svc or REST API ? > > All svc scripts that aren't meant for external use should be CSRFed. Any svc > scripts that are for external use should be replaced with REST API > endpoints. Based on my research and understanding, the REST API should > require a CSRF token *only* when using cookie based authentication, but not > when using token based authentication ( basic auth is a gray area where we > could possibly require CSRF but that would break it's ability to be used > outside Koha, which is the only time we use basic auth, so really it should > left as is with a stern warning ;) > > I think this is big work that is out of scope for the current project ( > which is already very big ), but would be a good followup. I agree that we should push this one then focus on svc (no bug report yet) and REST API (bug 34451). For the svc scripts there is actually no problem for now, none of them is using "op". However we have an example of how to implement it in svc/config/systempreferences and koha-tmpl/intranet-tmpl/prog/js/pages/preferences.js (In reply to David Cook from comment #105) > I just pushed some changes for "Administration > Z39.50/SRU servers". I > think I did the changes correctly, but let me know if there's any issue with > them. Seems correct. > I removed "cud-" from some GET view ops, converted the delete GET to POST > (using a Javascript-driven submit method I've used before, but not 100% sure > if that's the method we all want to use or not), Yes, it's what we are doing in other places. > and added cud-search for a > search POST (although perhaps I should've converted it to a GET instead?) It must be converted to GET, it's a stateless operation. (In reply to Jonathan Druart from comment #110) > (In reply to Marcel de Rooy from comment #102) > > We need some additional changes in C4/Auth (incl get_template_and_user) for > > the login process. Check the anonymous session token and the CSRF token. > > I skipped adding some cud- $op for auth.tt forms, since it felt awkward. But > > now the test looks at cud- and we do not check. > > > BUT I think we should actually enforce this condition (later or now): if > it's a POST then we must have a "op" parameter. In this case we will need to > discuss it the "login" case a bit more. Yes, thats my point. cud-login would not be my first choice ;) (In reply to Jonathan Druart from comment #110) > (In reply to Marcel de Rooy from comment #101) > > We are doing in C4/Auth.pm here: > > C4::Output::output_and_exit( $in->{query}, $cookie, $template, > > 'wrong_csrf_token' ); > > output_and_exit uses template variable blocking_error to pass it down. > > > > But many templates do not have the blocking_errors.inc and do not contain a > > blocking_error variable. So this does result in empty forms, confusion, etc? > > > > When testing changes, I did not yet see the error on my screen. Did you? > > With bug 34862 it should be everywhere now (unless exotic places like > modals). Please keep track of them. I do not see it everywhere. This was about OPAC forms.. It is not in opac templates?? I've added a commit to have the csrf token in a meta tag. It is useful when it needs to be retrived from JS for AJAX requests. (In reply to Marcel de Rooy from comment #112) > (In reply to Jonathan Druart from comment #110) > > (In reply to Marcel de Rooy from comment #101) > > > We are doing in C4/Auth.pm here: > > > C4::Output::output_and_exit( $in->{query}, $cookie, $template, > > > 'wrong_csrf_token' ); > > > output_and_exit uses template variable blocking_error to pass it down. > > > > > > But many templates do not have the blocking_errors.inc and do not contain a > > > blocking_error variable. So this does result in empty forms, confusion, etc? > > > > > > When testing changes, I did not yet see the error on my screen. Did you? > > > > With bug 34862 it should be everywhere now (unless exotic places like > > modals). Please keep track of them. > > I do not see it everywhere. This was about OPAC forms.. It is not in opac > templates?? Indeed, bug 34862 was covering the staff interface. I've pushed xt/find-missing-op-in-forms.t, it has 88 failures right now. (In reply to Jonathan Druart from comment #115) > I've pushed xt/find-missing-op-in-forms.t, it has 88 failures right now. Are we mainly focusing on https://annuel.framapad.org/p/koha_34478_missing_op at the moment or is there somewhere else I should be looking too? (In reply to David Cook from comment #116) > (In reply to Jonathan Druart from comment #115) > > I've pushed xt/find-missing-op-in-forms.t, it has 88 failures right now. > > Are we mainly focusing on > https://annuel.framapad.org/p/koha_34478_missing_op at the moment or is > there somewhere else I should be looking too? That's right, then we have the remaning (difficult) pad and probably a final run through Terrible regression: I have a keyword search defined in Firefox to search sysprefs from the address bar. I am getting "Programming error: 'search' must start with 'cud-' for POST" because Firefox posts the query. This means it will impacts librarians using keywords for specific searches. How can we workaround this? Could we allow some "op" for POST? Like "search"?.. *** Bug 32365 has been marked as a duplicate of this bug. *** *** Bug 32364 has been marked as a duplicate of this bug. *** *** Bug 32363 has been marked as a duplicate of this bug. *** *** Bug 32362 has been marked as a duplicate of this bug. *** *** Bug 32361 has been marked as a duplicate of this bug. *** *** Bug 32360 has been marked as a duplicate of this bug. *** *** Bug 32359 has been marked as a duplicate of this bug. *** (In reply to Jonathan Druart from comment #118) > Terrible regression: I have a keyword search defined in Firefox to search > sysprefs from the address bar. I am getting "Programming error: 'search' > must start with 'cud-' for POST" because Firefox posts the query. > > This means it will impacts librarians using keywords for specific searches. > How can we workaround this? Not sure if it is a terrible regression btw. This seems like an issue with Google keyword searches, not a Koha issue in the first place? Can imagine that not only Koha is hit by this behavior. How much additional script/template editing are we willing to do now for generating the expected search response? How much forms are possible candidates? (In reply to Marcel de Rooy from comment #127) > (In reply to Jonathan Druart from comment #118) > > Terrible regression: I have a keyword search defined in Firefox to search > > sysprefs from the address bar. I am getting "Programming error: 'search' > > must start with 'cud-' for POST" because Firefox posts the query. > > > > This means it will impacts librarians using keywords for specific searches. > > How can we workaround this? > > Not sure if it is a terrible regression btw. At least it is for me, I am using it dozen of times per day. > This seems like an issue with Google keyword searches, not a Koha issue in > the first place? It's Firefox. > Can imagine that not only Koha is hit by this behavior. Maybe not, but it's Koha I am using all the time :) > How much additional script/template editing are we willing to do now for > generating the expected search response? How much forms are possible > candidates? I don't know, but we could have one way to workaround it. Implement it for syspref search then add it case-by-case when needed. Not considering blocker for now however. Added bug 36084 - Pass CSRF token to SVC scripts For the svc/ajax scripts part. For discussion, waiting for feedback. (In reply to Marcel de Rooy from comment #111) > (In reply to Jonathan Druart from comment #110) > > (In reply to Marcel de Rooy from comment #102) > > > We need some additional changes in C4/Auth (incl get_template_and_user) for > > > the login process. Check the anonymous session token and the CSRF token. > > > I skipped adding some cud- $op for auth.tt forms, since it felt awkward. But > > > now the test looks at cud- and we do not check. > > > > > BUT I think we should actually enforce this condition (later or now): if > > it's a POST then we must have a "op" parameter. In this case we will need to > > discuss it the "login" case a bit more. > > Yes, thats my point. cud-login would not be my first choice ;) I think we need to treat login as other actions. It's a CUD operation, and "cud-login" is fine in my opinion. Happy to implement it if we agree on it. (In reply to Jonathan Druart from comment #130) > I think we need to treat login as other actions. It's a CUD operation, and > "cud-login" is fine in my opinion. Happy to implement it if we agree on it. Please continue. No blocker for me, just a bit awkward at first sight.. Created attachment 162114 [details] [review] Bug 34478: Do not allow absence of token Well, this test was silly, I was focussed on propagating an error to the UI, but we really need to explode in this case. Note that this requires more work as login is now broken. Pushed a fix for account-table.inc This form calls a payment plugin to prep a new form that submits to a the payment site external to Koha. This form does not change state but a change to GET would cause failures if trying to pay more than 133 fees on Chrome ( but not Firefox ). (In reply to Jonathan Druart from comment #132) > Note that this requires more work as login is now broken. Yeah I tried to add anti-CSRF to login before but I think we'd need to do the CSRF check earlier in the code to make it possible. As is, it's checking it for the wrong session when logging in. (In reply to Jonathan Druart from comment #118) > Terrible regression: I have a keyword search defined in Firefox to search > sysprefs from the address bar. I am getting "Programming error: 'search' > must start with 'cud-' for POST" because Firefox posts the query. https://wiki.koha-community.org/wiki/Koha_as_Firefox_Search_Engine Is that what you're doing? I don't think that you can do a POST as that would be a cross site request. > This means it will impacts librarians using keywords for specific searches. > How can we workaround this? I think that should usually be doing GET requests. -- Can you provide any more details on this one? We do have libraries that have HTML forms on other websites (like their intranet) that do form GETs to Koha. (In reply to Kyle M Hall from comment #133) > Pushed a fix for account-table.inc > > This form calls a payment plugin to prep a new form that submits to a > the payment site external to Koha. > > This form does not change state but a change to GET would cause failures > if trying to pay more than 133 fees on Chrome ( but not Firefox ). I'd argue that this might be stateful actually.. it depends on the plugin.. for example we have at least a couple of payment plugins that store a specially crafted token that's checked against with each payment redirect.. so we do in fact store something. I'm happy to replace the -stateless with something else and handle it in the controller... that also keeps the POST there for 133+ charges.. wow.. do patrons really reach this?! (In reply to David Cook from comment #135) > (In reply to Jonathan Druart from comment #118) > > Terrible regression: I have a keyword search defined in Firefox to search > > sysprefs from the address bar. I am getting "Programming error: 'search' > > must start with 'cud-' for POST" because Firefox posts the query. > > https://wiki.koha-community.org/wiki/Koha_as_Firefox_Search_Engine > > Is that what you're doing? > > I don't think that you can do a POST as that would be a cross site request. I am doing right click on a search input > "Add a keyword for this search" > > This means it will impacts librarians using keywords for specific searches. > > How can we workaround this? > > I think that should usually be doing GET requests. It's doing POST. > Can you provide any more details on this one? We do have libraries that have > HTML forms on other websites (like their intranet) that do form GETs to Koha. I don't understand the question. What are the forms about? stateless? If so it will still work as we are converting stateless forms to GET. (In reply to Martin Renvoize from comment #136) > (In reply to Kyle M Hall from comment #133) > > Pushed a fix for account-table.inc > > > > This form calls a payment plugin to prep a new form that submits to a > > the payment site external to Koha. > > > > This form does not change state but a change to GET would cause failures > > if trying to pay more than 133 fees on Chrome ( but not Firefox ). > > I'd argue that this might be stateful actually.. it depends on the plugin.. > for example we have at least a couple of payment plugins that store a > specially crafted token that's checked against with each payment redirect.. > so we do in fact store something. > > I'm happy to replace the -stateless with something else and handle it in the > controller... that also keeps the POST there for 133+ charges.. wow.. do > patrons really reach this?! "cud-stateless" does not seem correct indeed ;) (In reply to Jonathan Druart from comment #137 > > I don't understand the question. What are the forms about? stateless? If so > it will still work as we are converting stateless forms to GET. We can't convert to GET, because the URL will become too long when the 130 lines are submitted as params, internet seems to largely agree ~2000 will work in mosy browsers (In reply to David Cook from comment #107) > (In reply to Marcel de Rooy from comment #102) > > This one is important: > > > > We need some additional changes in C4/Auth (incl get_template_and_user) for > > the login process. Check the anonymous session token and the CSRF token. > > I skipped adding some cud- $op for auth.tt forms, since it felt awkward. But > > now the test looks at cud- and we do not check. > > > > Any thoughts? > > I noticed for auth.tt and opac-auth.tt that the CSRF token wasn't being > generated for anonymous users, because the sessionID wasn't being passed to > the template. I've fixed that. > > Same goes for anonymous users and /cgi-bin/koha/installer/install.pl. Fixing > that one too. > > Note the fix is just for CSRF token generation. Not the anti-CSRF work > itself. Well, I forgot that. It's now on bug 36092. Remote branch rebased on top of the dependent bugs. Created attachment 162165 [details] [review] Bug 34478: (follow-up) Change invoice files delete link to POST Comment on attachment 162165 [details] [review] Bug 34478: (follow-up) Change invoice files delete link to POST Added to the remote branch. (In reply to Nick Clemens from comment #139) > (In reply to Jonathan Druart from comment #137 > > > > I don't understand the question. What are the forms about? stateless? If so > > it will still work as we are converting stateless forms to GET. > > We can't convert to GET, because the URL will become too long when the 130 > lines are submitted as params, internet seems to largely agree ~2000 will > work in mosy browsers We will certainly hit this limitation in different places. I am wondering if we should not have (yet another) trick for them. We could have another prefix to identify them? op =~ /^st-/ (st for stateless)? (In reply to Jonathan Druart from comment #144) > (In reply to Nick Clemens from comment #139) > > (In reply to Jonathan Druart from comment #137 > > > > > > I don't understand the question. What are the forms about? stateless? If so > > > it will still work as we are converting stateless forms to GET. > > > > We can't convert to GET, because the URL will become too long when the 130 > > lines are submitted as params, internet seems to largely agree ~2000 will > > work in mosy browsers > > We will certainly hit this limitation in different places. > > I am wondering if we should not have (yet another) trick for them. We could > have another prefix to identify them? op =~ /^st-/ (st for stateless)? Wouldnt that be a security hole ? I'm confused... what's the issue? For Kyle's thing, why don't we just use POST, require a CSRF token, and use an "op" of "cud-pay". I don't think we need to get too hung up on stateful vs stateless. A stateful change needs to be done with a POST with a CSRF token to prevent a CSRF attack. It's OK for a stateless change to be done by a POST, but we will still require it to have a CSRF token, because all POSTs need them to provide an umbrella of security across the app. (In reply to David Cook from comment #146) > I'm confused... what's the issue? > > For Kyle's thing, why don't we just use POST, require a CSRF token, and use > an "op" of "cud-pay". It's what has been done. > I don't think we need to get too hung up on stateful vs stateless. > > A stateful change needs to be done with a POST with a CSRF token to prevent > a CSRF attack. It's OK for a stateless change to be done by a POST, but we > will still require it to have a CSRF token, because all POSTs need them to > provide an umbrella of security across the app. Yes. My suggestion was to distinguish stateless and stateful for POST, to avoid "cud-stateless", "cud-show", "cud-search". They do not make sense. It's just wording. (In reply to Marcel de Rooy from comment #145) > (In reply to Jonathan Druart from comment #144) > > (In reply to Nick Clemens from comment #139) > > > (In reply to Jonathan Druart from comment #137 > > > > > > > > I don't understand the question. What are the forms about? stateless? If so > > > > it will still work as we are converting stateless forms to GET. > > > > > > We can't convert to GET, because the URL will become too long when the 130 > > > lines are submitted as params, internet seems to largely agree ~2000 will > > > work in mosy browsers > > > > We will certainly hit this limitation in different places. > > > > I am wondering if we should not have (yet another) trick for them. We could > > have another prefix to identify them? op =~ /^st-/ (st for stateless)? > > Wouldnt that be a security hole ? Why that? I don't think so if it's handled correctly. Btw troubleshooting auth issues would be easier if we had bug 36034 in this branch. I haven't done any RM/RMaint for Koha community. Do we need to rebase against master, cherry-pick, or merge those commits? (In reply to David Cook from comment #149) > Btw troubleshooting auth issues would be easier if we had bug 36034 in this > branch. Added to the branch. Bug 34778: Manual fix - Make Koha::Token use session id not userenv id David, I don't understand what you are doing here, it is missing context and a proper commit message. We have session_id in the $params already. You are fetching the session once again but it's not necessary. Also now we have id="${session_id}_${session_id}", it feels... weird. In which case don't we have a session? We should always have a session (at least for now). Watchout the bug number in the commit title please. Fixing the following commits: David Cook Bug 34778: Manual fix - Make Koha::Token use session id not userenv id David Cook Bug 34778: Manual fix - Fix controller op handling of self-checkout David Cook Bug 34778: Manual fix - Fix session cookie management of self-checkout David Cook Bug 34778: Manual fix - Remove csrf_token from GET to sco-patron-image.pl David Cook Bug 34778: Manual fix - Remove cud-search from z3950-admin-search.inc David Cook Revert "Bug 34778: Manual fix - pass sessionID to installer/auth.tt" David Cook Bug 34778: Manual fix - pass sessionID to installer/auth.tt David Cook Bug 34778: Manual fix - Add cud-search to z3950-admin-search.inc David Cook Bug 34778: Manual fix - Convert Z39.50 delete from GET to POST David Cook Bug 34778: Manual fix - fix add/edit/copy display of Z39.50 servers Martin Renvoize Bug 34778: Manual fix - add op - pos/pay Lucas Gass Bug 34778: Manual fix - add op - rotating_collections/addItems Lucas Gass Bug 34778: Manual fix - add op - rotating_collections/transferCollection Kyle M Hall Bug 34778: Manual fix - add op - members/housebound Kyle M Hall Bug 34778: Manual fix - add op - members/maninvoice Kyle M Hall Bug 34778: Manual fix - add op - members/mancredit Martin Renvoize Bug 34778: Manual fix - add op - members/member-password Martin Renvoize Bug 34778: Manual fix - add op - members/discharge Marcel de Rooy Bug 34778: Changes for opac-sendshelf The fundamental of the approach: having the CSRF check in CGI->new is TERRIBLY WRONG. It only works because we are using one user/one session, but things will be terrible with several users using Koha. C4::Context userenv is in plack worker memory, and it's only why the CSRF check is (more or less) working. David kind of caught it suggesting to use session_id instead of userenv's id (see commit "Manual fix - Make Koha::Token use session id not userenv id"), but I don't think it's enough. David suggested to write a Middleware for that, should we have it from the start? About userenv still in worker's memory, is it a security problem? (to report separately?) Several things to discuss here, and I am feeling a bit alone, waiting everyday to catch David's replies and get other questions... See also bug 36102 comment 34. (In reply to Jonathan Druart from comment #153) > C4::Context userenv is in plack worker memory, and it's only why the CSRF > check is (more or less) working. Could be the cause of bug 30109? (Nick pointed me that one) (In reply to Jonathan Druart from comment #151) > Bug 34778: Manual fix - Make Koha::Token use session id not userenv id > > David, I don't understand what you are doing here, it is missing context and > a proper commit message. > We have session_id in the $params already. You are fetching the session once > again but it's not necessary. > Also now we have id="${session_id}_${session_id}", it feels... weird. > > In which case don't we have a session? We should always have a session (at > least for now). My apologies. One of those things where I probably spent so much time on it that I forgot that it wasn't self-evident on its own. We have session_id in the $params, but not the "id" which is not the session id but the user id. (session_id is $session->id() but "id" is $session->param('id'). Subtle difference but very significant.) id = "dcook_2408151i24tb124l6gt12" or id = "anonymous_32uig4624lig24g624lg" (In reply to Jonathan Druart from comment #152) > Watchout the bug number in the commit title please. Ugh, I'm sorry about that! I'll do my best to do better there. (In reply to Jonathan Druart from comment #153) > The fundamental of the approach: having the CSRF check in CGI->new is > TERRIBLY WRONG. > It only works because we are using one user/one session, but things will be > terrible with several users using Koha. > > C4::Context userenv is in plack worker memory, and it's only why the CSRF > check is (more or less) working. > > David kind of caught it suggesting to use session_id instead of userenv's id > (see commit "Manual fix - Make Koha::Token use session id not userenv id"), > but I don't think it's enough. That sounds bad. I haven't tested it, but sounds bad. Although I don't think I see userenv being used in the generation/checking at the moment? The change I made to lookup userid via the session should make this dependent on the per-request cookie I believe. > David suggested to write a Middleware for that, should we have it from the > start? There is a bit of a learning curve for writing the Middleware, but it would be a fairly simple Middleware. > About userenv still in worker's memory, is it a security problem? (to report > separately?) Hmm might be yeah. It's worth investigating at the very least. (It's also one reason why I find multitenanted Koha where different libraries are supplied by the same backend web server a bit scary.) > Several things to discuss here, and I am feeling a bit alone, waiting > everyday to catch David's replies and get other questions... Sorry to hear you've been feeling alone. I've appreciated your work and messages, and I want you to know I think you've been doing great on this Herculean task. > See also bug 36102 comment 34. Going to go look at it now. (In reply to David Cook from comment #157) > (In reply to Jonathan Druart from comment #153) > > David suggested to write a Middleware for that, should we have it from the > > start? > > There is a bit of a learning curve for writing the Middleware, but it would > be a fairly simple Middleware. I failed, not trivial, see bug 36148. (In reply to David Cook from comment #155) > (In reply to Jonathan Druart from comment #151) > > Bug 34778: Manual fix - Make Koha::Token use session id not userenv id > > > > David, I don't understand what you are doing here, it is missing context and > > a proper commit message. > > We have session_id in the $params already. You are fetching the session once > > again but it's not necessary. > > Also now we have id="${session_id}_${session_id}", it feels... weird. > > > > In which case don't we have a session? We should always have a session (at > > least for now). > > My apologies. One of those things where I probably spent so much time on it > that I forgot that it wasn't self-evident on its own. > > We have session_id in the $params, but not the "id" which is not the session > id but the user id. (session_id is $session->id() but "id" is > $session->param('id'). Subtle difference but very significant.) > > id = "dcook_2408151i24tb124l6gt12" > > or > > id = "anonymous_32uig4624lig24g624lg" I definitely misread and got confused with $session->id vs $session->param('id')! All good then (even if I will still be confused by this one in the future!) (In reply to Jonathan Druart from comment #153) > About userenv still in worker's memory, is it a security problem? (to report > separately?) Opened bug 36149. Was testing this a bit (far too late), but just posting it here:
Adding this opac-bottom.inc might (have) drastically reduce(d) the number of token includes in our templates?
+ $('form').on( 'submit', function(e) {
+ if( $(this).attr('method').toUpperCase() == 'POST' ) {
+ var csrf_token = $("meta[name='csrf-token']").attr('content');
+ if( !csrf_token ) return false;
+ $(this).append( '<input type="hidden" name="csrf_token" value="' + csrf_token + '" />' );
+ }
+ });
(In reply to Marcel de Rooy from comment #161) > Was testing this a bit (far too late), but just posting it here: > > Adding this opac-bottom.inc might (have) drastically reduce(d) the number of > token includes in our templates? > > + $('form').on( 'submit', function(e) { > + if( $(this).attr('method').toUpperCase() == 'POST' ) { > + var csrf_token = $("meta[name='csrf-token']").attr('content'); > + if( !csrf_token ) return false; > + $(this).append( '<input type="hidden" name="csrf_token" > value="' + csrf_token + '" />' ); > + } > + }); Yes, but most of the INCLUDEs have been added by a script and... the token in the meta tag arrived later in the process/thought. (In reply to Marcel de Rooy from comment #161) > Was testing this a bit (far too late), but just posting it here: > > Adding this opac-bottom.inc might (have) drastically reduce(d) the number of > token includes in our templates? > > + $('form').on( 'submit', function(e) { > + if( $(this).attr('method').toUpperCase() == 'POST' ) { > + var csrf_token = $("meta[name='csrf-token']").attr('content'); > + if( !csrf_token ) return false; > + $(this).append( '<input type="hidden" name="csrf_token" > value="' + csrf_token + '" />' ); > + } > + }); This could still be done. It also means that we won't generate different CSRF tokens for the same request. Remote branch cleaned. I removed the "DO NOT PUSH" and squashed the "TO SQUASH". (In reply to Jonathan Druart from comment #164) > Remote branch cleaned. I removed the "DO NOT PUSH" and squashed the "TO > SQUASH". backup of the old version security/old/20240222-bug_34478 (In reply to Marcel de Rooy from comment #161) > Was testing this a bit (far too late), but just posting it here: > > Adding this opac-bottom.inc might (have) drastically reduce(d) the number of > token includes in our templates? > > + $('form').on( 'submit', function(e) { > + if( $(this).attr('method').toUpperCase() == 'POST' ) { > + var csrf_token = $("meta[name='csrf-token']").attr('content'); > + if( !csrf_token ) return false; > + $(this).append( '<input type="hidden" name="csrf_token" > value="' + csrf_token + '" />' ); > + } > + }); I think the norm is to insert the tokens into the HTML forms using the server-side templates, but this does have a certain elegance to it. (In reply to Jonathan Druart from comment #162) > Yes, but most of the INCLUDEs have been added by a script and... the token > in the meta tag arrived later in the process/thought. It's certainly been a learning process for everyone, eh? Btw it looks like bug_34478 branch is missing Koha::Middleware::CSRF but it's plack.psgi references it, so this branch is a bit broken atm. (In reply to Jonathan Druart from comment #163) > This could still be done. It also means that we won't generate different > CSRF tokens for the same request. It looks like my previous idea of saving it in in the Koha::Template::Plugin::Koha object won't work as each [% INCLUDE 'csrf-token.inc' %] creates a new Koha object. I thought that it would load the Koha::Template::Plugin::Koha module once, and then create an object once, but it doesn't appear that way. According to http://template-toolkit.org/docs/manual/Directives.html#section_USE [% USE MyPlugin('foo', 123) %] is equivalent to Template::Plugin::MyPlugin->new($context, 'foo', 123); (In reply to David Cook from comment #169) > (In reply to Jonathan Druart from comment #163) > > This could still be done. It also means that we won't generate different > > CSRF tokens for the same request. > > It looks like my previous idea of saving it in in the > Koha::Template::Plugin::Koha object won't work as each [% INCLUDE > 'csrf-token.inc' %] creates a new Koha object. But we can still use Koha::Cache::Memory::Lite like we do in other Koha::Template::Plugin::* modules (In reply to David Cook from comment #170) > (In reply to David Cook from comment #169) > > (In reply to Jonathan Druart from comment #163) > > > This could still be done. It also means that we won't generate different > > > CSRF tokens for the same request. > > > > It looks like my previous idea of saving it in in the > > Koha::Template::Plugin::Koha object won't work as each [% INCLUDE > > 'csrf-token.inc' %] creates a new Koha object. > But we can still use Koha::Cache::Memory::Lite like we do in other > Koha::Template::Plugin::* modules I've just pushed a patch that does this to bug_34478 (In reply to David Cook from comment #168) > Btw it looks like bug_34478 branch is missing Koha::Middleware::CSRF but > it's plack.psgi references it, so this branch is a bit broken atm. I don't understand, no it's not. Koha::Middleware::CSRF later in the tree and there is no reference to it in bug_34478. (In reply to David Cook from comment #171) > (In reply to David Cook from comment #170) > > (In reply to David Cook from comment #169) > > > (In reply to Jonathan Druart from comment #163) > > > > This could still be done. It also means that we won't generate different > > > > CSRF tokens for the same request. > > > > > > It looks like my previous idea of saving it in in the > > > Koha::Template::Plugin::Koha object won't work as each [% INCLUDE > > > 'csrf-token.inc' %] creates a new Koha object. > > But we can still use Koha::Cache::Memory::Lite like we do in other > > Koha::Template::Plugin::* modules > > I've just pushed a patch that does this to bug_34478 Yes, it's a good one. However I think it needs its own bug (and tests). > (In reply to David Cook from comment #171) > > (In reply to David Cook from comment #170) > > > (In reply to David Cook from comment #169) > > > > (In reply to Jonathan Druart from comment #163) > > > > > This could still be done. It also means that we won't generate different > > > > > CSRF tokens for the same request. > > > > > > > > It looks like my previous idea of saving it in in the > > > > Koha::Template::Plugin::Koha object won't work as each [% INCLUDE > > > > 'csrf-token.inc' %] creates a new Koha object. > > > But we can still use Koha::Cache::Memory::Lite like we do in other > > > Koha::Template::Plugin::* modules > > > > I've just pushed a patch that does this to bug_34478 > > Yes, it's a good one. However I think it needs its own bug (and tests). And we have already one: bug 35955. Marcel, on the branch bug_34478_svc t/Token.t is failing with:
# Failed test 'CSRF token is not verified if another logged in user is using the same id'
# at t/Token.t line 76.
# got: '1'
# expected: ''
# Looks like you failed 1 test of 2.
# Failed test 'Same id (cookie CGISESSID) with an other logged in user'
# at t/Token.t line 77.
# Failed test 'Check userid'
# at t/Token.t line 128.
# got: 'anonymous_567'
# expected: '42_567'
# Looks like you failed 1 test of 8.
I was actually thinking about that this morning, and the tests caught it.
We are more permissive in the CSRF check, but it's only a facade as we are filling the userenv from the session.
Can you please clarify or confirm? I am about to simply remove the first test and adjust the second one.
(In reply to Jonathan Druart from comment #174) > Marcel, on the branch bug_34478_svc t/Token.t is failing with: > > # Failed test 'CSRF token is not verified if another logged in user is > using the same id' > # at t/Token.t line 76. > # got: '1' > # expected: '' > # Looks like you failed 1 test of 2. > > # Failed test 'Same id (cookie CGISESSID) with an other logged in user' > # at t/Token.t line 77. > > # Failed test 'Check userid' > # at t/Token.t line 128. > # got: 'anonymous_567' > # expected: '42_567' > # Looks like you failed 1 test of 8. > > > I was actually thinking about that this morning, and the tests caught it. > > We are more permissive in the CSRF check, but it's only a facade as we are > filling the userenv from the session. > > Can you please clarify or confirm? I am about to simply remove the first > test and adjust the second one. I wont have time for this right now. So go ahead and make the needed changes. (In reply to Jonathan Druart from comment #174) > Marcel, on the branch bug_34478_svc t/Token.t is failing with: > > # Failed test 'CSRF token is not verified if another logged in user is > using the same id' > # at t/Token.t line 76. > # got: '1' > # expected: '' > # Looks like you failed 1 test of 2. > > # Failed test 'Same id (cookie CGISESSID) with an other logged in user' > # at t/Token.t line 77. > > # Failed test 'Check userid' > # at t/Token.t line 128. > # got: 'anonymous_567' > # expected: '42_567' > # Looks like you failed 1 test of 8. > > > I was actually thinking about that this morning, and the tests caught it. > > We are more permissive in the CSRF check, but it's only a facade as we are > filling the userenv from the session. > > Can you please clarify or confirm? I am about to simply remove the first > test and adjust the second one. Tests removed and adjusted see commit "Bug 34478: (follow-up) Manual fix - Make Koha::Token use session id not userenv id". Please provide a follow-up if needed. (In reply to Jonathan Druart from comment #174) > I was actually thinking about that this morning, and the tests caught it. > > We are more permissive in the CSRF check, but it's only a facade as we are > filling the userenv from the session. > > Can you please clarify or confirm? I am about to simply remove the first > test and adjust the second one. Looking at t/Token.t and I think we could remove the following tests: 'Same id (cookie CGISESSID) with an other logged in user' 'Same logged in user with another session (cookie CGISESSID)' 'testing _add_default_csrf_params with/without userenv (bug 27849)' They don't really add any value. We'd need to add tests to show that token params are created correctly for anonymous users and authenticated users (just in terms of sessions - don't need to worry about userenv). Created attachment 162630 [details] [review] Bug 34478: (follow-up) Move resend link out of form for display reasons This improves display. This only comes up when you try to reset your password after you did already. (In reply to Marcel de Rooy from comment #178) > Created attachment 162630 [details] [review] [review] > Bug 34478: (follow-up) Move resend link out of form for display reasons > > This improves display. This only comes up when you try to reset your > password after you did already. I cant push anymore ;) Comment on attachment 162630 [details] [review] Bug 34478: (follow-up) Move resend link out of form for display reasons Picked for today's push. Pushed to master for 24.05.00. Not for backport I bet ? (In reply to Fridolin Somers from comment #182) > Not for backport I bet ? I don't think that it would be possible to backport this one. However, I have been thinking a bit about how to provide some protection to stable branches. Locally, I've applied the following: - Bug 36098 (the Koha::Session patches) - Bug 34755: Backport Koha::Token change from bug 34478 - Bug 34478: Add csrf-token in meta I've created a middleware based off Koha::Middleware::CSRF and then using a mix of ideas from Marcel and myself, I've used Javascript to inject CSRF tokens into forms and Koha API calls. At the moment, I'm testing this on the OPAC, and then I'm going to look at the Staff Interface. I want to do some more thinking about how we can use "Strict" in the SameSite attribute for the CGISESSID cookie to cover off CSRF for GET requests as well. Less of a problem for 34478 because it fixes a lot of stateful GET requests, but for older versions... -- Long story short... it might be worth backporting just "Bug 34478: Add csrf-token in meta" for now. Maybe a new bug report for that? (In reply to David Cook from comment #183) > However, I have been thinking a bit about how to provide some protection to > stable branches. I think my abbreviated anti-CSRF plus bug 33259 (when it's ready) would be an OK compromise for stable branches. (In reply to David Cook from comment #184) > (In reply to David Cook from comment #183) > > However, I have been thinking a bit about how to provide some protection to > > stable branches. > > I think my abbreviated anti-CSRF plus bug 33259 (when it's ready) would be > an OK compromise for stable branches. Although as I say that I'm finding more changes that need to be backported to really support this... heh... OK thanks for your comments. For the moment no backport *** Bug 35677 has been marked as a duplicate of this bug. *** *** Bug 19645 has been marked as a duplicate of this bug. *** *** Bug 22314 has been marked as a duplicate of this bug. *** *** Bug 23060 has been marked as a duplicate of this bug. *** *** Bug 34111 has been marked as a duplicate of this bug. *** *** Bug 23238 has been marked as a duplicate of this bug. *** *** Bug 30502 has been marked as a duplicate of this bug. *** (In reply to David Cook from comment #185) > Although as I say that I'm finding more changes that need to be backported > to really support this... heh... Like koha-tmpl/intranet-tmpl/prog/js/file-upload.js uses XMLHttpRequest directly and doesn't do AJAX calls via jQuery. |