Bug 34478 - Full CSRF protection
Summary: Full CSRF protection
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P1 - high blocker (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
: 19645 22314 23060 23238 30502 32359 32360 32361 32362 32363 32364 32365 34111 35677 (view as bug list)
Depends on: 30524 34426 35890 35935 35951 35955 36092 36098 36017 36019
Blocks: 34608 35992 36189 36192 36195 36207 36577 36630 36102 36148 36190 36193 36219 36237 36241 36243 36274 36280 36326 36336 36349 36351 36368 36384 36568 36683
  Show dependency treegraph
 
Reported: 2023-08-04 06:59 UTC by Jonathan Druart
Modified: 2024-04-24 15:31 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00


Attachments
Bug 34478: Script to catch GET forms (9.92 KB, patch)
2023-08-04 09:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Replace get with post when needed (12.95 KB, patch)
2023-08-04 09:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Replace csrf_token input with include file (19.30 KB, patch)
2023-08-04 09:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Replace csrf_token input with include file - manual (2.68 KB, patch)
2023-08-04 09:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Remove generate_csrf from pl (15.37 KB, patch)
2023-08-04 09:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Add a script to add missing csrf to post forms - csrf_add_missing_csrf (2.56 KB, patch)
2023-08-04 09:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Add missing CSRF token to POST forms (380.44 KB, patch)
2023-08-04 09:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Add a script to catch missing 'op' in POST forms - catch_missing_op (2.34 KB, patch)
2023-08-04 09:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Output of catch_missing_op.pl (11.24 KB, patch)
2023-08-04 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Add 'op' to opac-user.tt (2.63 KB, patch)
2023-08-04 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Add 'op' to opac-passwd (1.86 KB, patch)
2023-08-04 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Adjust missing_op.txt (976 bytes, patch)
2023-08-04 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Check CSRF in get_template_and_user (1.95 KB, patch)
2023-08-04 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Remove check_csrf from pl files (16.02 KB, patch)
2023-08-04 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Script to catch GET forms (9.92 KB, patch)
2023-09-21 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Replace get with post when needed (12.95 KB, patch)
2023-09-21 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Replace csrf_token input with include file (19.35 KB, patch)
2023-09-21 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Replace csrf_token input with include file - manual (2.69 KB, patch)
2023-09-21 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Remove generate_csrf from pl (15.37 KB, patch)
2023-09-21 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Add a script to add missing csrf to post forms - csrf_add_missing_csrf (2.56 KB, patch)
2023-09-21 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Add missing CSRF token to POST forms (379.84 KB, patch)
2023-09-21 10:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Add a script to catch missing 'op' in POST forms - catch_missing_op (2.34 KB, patch)
2023-09-21 10:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Output of catch_missing_op.pl (11.24 KB, patch)
2023-09-21 10:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Add 'op' to opac-user.tt (2.63 KB, patch)
2023-09-21 10:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Add 'op' to opac-passwd (1.86 KB, patch)
2023-09-21 10:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Adjust missing_op.txt (976 bytes, patch)
2023-09-21 10:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Check CSRF in get_template_and_user (1.95 KB, patch)
2023-09-21 10:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Remove check_csrf from pl files (16.07 KB, patch)
2023-09-21 10:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Move C4::Auth check (3.42 KB, patch)
2023-09-21 10:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: op-cud - Adjust C4::Auth code (948 bytes, patch)
2023-09-21 10:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: op-cud - Add a script to rename op with op-cud in templates (2.07 KB, patch)
2023-09-21 10:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: op-cud - Rename op with op-cud in templates (249.08 KB, patch)
2023-09-21 10:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: op-cud - Trick CGI directly (3.44 KB, patch)
2023-09-21 10:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: op-cud - Trick CGI directly (5.91 KB, patch)
2023-09-21 12:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: op =~ ^cud- (6.43 KB, patch)
2023-09-22 11:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: [DO NOT PUSH] Cypress tests - yarn add cypress-mysql (8.33 KB, patch)
2023-09-22 11:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: [DO NOT PUSH] Cypress tests - cypress-mysql config (1.75 KB, patch)
2023-09-22 11:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: [DO NOT PUSH] Cypress tests (5.63 KB, patch)
2023-09-22 11:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: [DO NOT PUSH] Cypress tests (5.59 KB, patch)
2023-09-22 11:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: Do not allow absence of token (2.32 KB, patch)
2024-02-13 15:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34478: (follow-up) Change invoice files delete link to POST (2.50 KB, patch)
2024-02-14 18:57 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 34478: (follow-up) Move resend link out of form for display reasons (2.19 KB, patch)
2024-03-01 07:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2023-08-04 06:59:29 UTC
This is for discussion, yet another bug report.

All this is a nightmare to fix, we have too many controllers.

We need to fix:
1. pl/tt
2. REST API (bug 34451)
3. svc scripts
4. ajax request from .js files (?)

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.

2. See bug 34451 comment 1

3 and 4: I think they are going together, we need to go through the svc scripts. There are not much and can be done manually.
Comment 1 Jonathan Druart 2023-08-04 07:24:47 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.
Comment 2 Marcel de Rooy 2023-08-04 07:41:07 UTC
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 ;)
Comment 3 Jonathan Druart 2023-08-04 07:52:17 UTC
(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.
Comment 4 Jonathan Druart 2023-08-04 09:48:32 UTC
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
Comment 5 Jonathan Druart 2023-08-04 09:48:35 UTC
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"
Comment 6 Jonathan Druart 2023-08-04 09:48:38 UTC
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"
Comment 7 Jonathan Druart 2023-08-04 09:48:41 UTC
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)
Comment 8 Jonathan Druart 2023-08-04 09:48:45 UTC
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
Comment 9 Jonathan Druart 2023-08-04 09:48:49 UTC
Created attachment 154246 [details] [review]
Bug 34478: Add a script to add missing csrf to post forms - csrf_add_missing_csrf
Comment 10 Jonathan Druart 2023-08-04 09:48:53 UTC
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
Comment 11 Jonathan Druart 2023-08-04 09:48:56 UTC
Created attachment 154248 [details] [review]
Bug 34478: Add a script to catch missing 'op' in POST forms - catch_missing_op
Comment 12 Jonathan Druart 2023-08-04 09:49:00 UTC
Created attachment 154249 [details] [review]
Bug 34478: Output of catch_missing_op.pl

% perl catch_missing_op.pl > missing_op.txt
Comment 13 Jonathan Druart 2023-08-04 09:49:05 UTC
Created attachment 154250 [details] [review]
Bug 34478: Add 'op' to opac-user.tt
Comment 14 Jonathan Druart 2023-08-04 09:49:10 UTC
Created attachment 154251 [details] [review]
Bug 34478: Add 'op' to opac-passwd
Comment 15 Jonathan Druart 2023-08-04 09:49:13 UTC
Created attachment 154252 [details] [review]
Bug 34478: Adjust missing_op.txt

To show what we have done in the previous "Add 'op' to " commits
Comment 16 Jonathan Druart 2023-08-04 09:49:17 UTC
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
Comment 17 Jonathan Druart 2023-08-04 09:49:20 UTC
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
Comment 18 Jonathan Druart 2023-08-04 09:52:04 UTC
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.
Comment 19 David Cook 2023-08-08 01:35:42 UTC
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
Comment 20 David Cook 2023-08-08 02:11:37 UTC
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).
Comment 21 David Cook 2023-08-08 02:39:36 UTC
(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.
Comment 22 David Cook 2023-08-08 02:52:01 UTC
(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.
Comment 23 David Cook 2023-08-08 02:53:47 UTC
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.
Comment 24 Marcel de Rooy 2023-08-09 14:05:39 UTC
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' %]
Comment 25 David Cook 2023-09-21 01:12:39 UTC
(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.
Comment 26 Jonathan Druart 2023-09-21 06:42:43 UTC
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.
Comment 27 Jonathan Druart 2023-09-21 10:04:33 UTC
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
Comment 28 Jonathan Druart 2023-09-21 10:04:37 UTC
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"
Comment 29 Jonathan Druart 2023-09-21 10:04:41 UTC
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"
Comment 30 Jonathan Druart 2023-09-21 10:04:45 UTC
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)
Comment 31 Jonathan Druart 2023-09-21 10:04:49 UTC
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
Comment 32 Jonathan Druart 2023-09-21 10:04:53 UTC
Created attachment 155976 [details] [review]
Bug 34478: Add a script to add missing csrf to post forms - csrf_add_missing_csrf
Comment 33 Jonathan Druart 2023-09-21 10:04:57 UTC
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
Comment 34 Jonathan Druart 2023-09-21 10:05:01 UTC
Created attachment 155978 [details] [review]
Bug 34478: Add a script to catch missing 'op' in POST forms - catch_missing_op
Comment 35 Jonathan Druart 2023-09-21 10:05:05 UTC
Created attachment 155979 [details] [review]
Bug 34478: Output of catch_missing_op.pl

% perl catch_missing_op.pl > missing_op.txt
Comment 36 Jonathan Druart 2023-09-21 10:05:08 UTC
Created attachment 155980 [details] [review]
Bug 34478: Add 'op' to opac-user.tt
Comment 37 Jonathan Druart 2023-09-21 10:05:12 UTC
Created attachment 155981 [details] [review]
Bug 34478: Add 'op' to opac-passwd
Comment 38 Jonathan Druart 2023-09-21 10:05:15 UTC
Created attachment 155982 [details] [review]
Bug 34478: Adjust missing_op.txt

To show what we have done in the previous "Add 'op' to " commits
Comment 39 Jonathan Druart 2023-09-21 10:05:19 UTC
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
Comment 40 Jonathan Druart 2023-09-21 10:05:23 UTC
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
Comment 41 Jonathan Druart 2023-09-21 10:05:26 UTC
Created attachment 155985 [details] [review]
Bug 34478: Move C4::Auth check
Comment 42 Jonathan Druart 2023-09-21 10:05:30 UTC
Created attachment 155986 [details] [review]
Bug 34478: op-cud - Adjust C4::Auth code
Comment 43 Jonathan Druart 2023-09-21 10:05:33 UTC
Created attachment 155987 [details] [review]
Bug 34478: op-cud - Add a script to rename op with op-cud in templates
Comment 44 Jonathan Druart 2023-09-21 10:05:38 UTC
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
Comment 45 Jonathan Druart 2023-09-21 10:05:41 UTC
Created attachment 155989 [details] [review]
Bug 34478: op-cud - Trick CGI directly
Comment 46 Jonathan Druart 2023-09-21 10:07:06 UTC
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.
Comment 47 Jonathan Druart 2023-09-21 11:18:31 UTC
Wait a bit more here.
Comment 48 Jonathan Druart 2023-09-21 12:03:44 UTC
Created attachment 155990 [details] [review]
Bug 34478: op-cud - Trick CGI directly
Comment 49 Jonathan Druart 2023-09-21 13:15:40 UTC
Ideally we need bug 34862 to be pushed before this.
Comment 50 Jonathan Druart 2023-09-21 14:22:19 UTC
Well, I am missing the obvious here... We can GET with op and without CSRF token, and we pass through everything (lol!)
Comment 51 Jonathan Druart 2023-09-21 14:48:24 UTC
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...
Comment 52 David Cook 2023-09-22 00:14:15 UTC
(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.
Comment 53 David Cook 2023-09-22 00:23:37 UTC
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.
Comment 54 David Cook 2023-09-22 01:18:07 UTC
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.
Comment 55 David Cook 2023-09-22 02:27:46 UTC
./admin/import_export_framework.pl is already separating the GET and POST handling (although it doesn't have any error handling).
Comment 56 David Cook 2023-09-22 02:40:05 UTC
(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.
Comment 57 David Cook 2023-09-22 02:46:10 UTC
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...
Comment 58 Jonathan Druart 2023-09-22 06:19:41 UTC
(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?
Comment 59 Marcel de Rooy 2023-09-22 06:24:22 UTC
We need some more coordination here. Otherwise we are spending valuable time without getting much further.
Comment 60 David Cook 2023-09-22 07:00:57 UTC
(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.
Comment 61 Jonathan Druart 2023-09-22 11:38:14 UTC
Created attachment 156070 [details] [review]
Bug 34478: op =~ ^cud-
Comment 62 Jonathan Druart 2023-09-22 11:38:17 UTC
Created attachment 156071 [details] [review]
Bug 34478: [DO NOT PUSH] Cypress tests - yarn add cypress-mysql
Comment 63 Jonathan Druart 2023-09-22 11:38:21 UTC
Created attachment 156072 [details] [review]
Bug 34478: [DO NOT PUSH] Cypress tests - cypress-mysql config
Comment 64 Jonathan Druart 2023-09-22 11:38:25 UTC
Created attachment 156073 [details] [review]
Bug 34478: [DO NOT PUSH] Cypress tests

This should not be pushed, we are not ready on jenkins.
Comment 65 Jonathan Druart 2023-09-22 11:38:45 UTC
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?
Comment 66 Jonathan Druart 2023-09-22 11:43:43 UTC
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.
Comment 67 Jonathan Druart 2023-09-22 11:46:39 UTC
Created attachment 156074 [details] [review]
Bug 34478: [DO NOT PUSH] Cypress tests

This should not be pushed, we are not ready on jenkins.
Comment 68 David Cook 2023-09-25 00:35:42 UTC
(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.
Comment 69 Jonathan Druart 2023-09-26 09:51:43 UTC
(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).
Comment 70 Jonathan Druart 2023-10-13 07:02:41 UTC
Without feedback and help I will abandon this one in the next couple of weeks.
Comment 71 Kyle M Hall 2023-10-13 16:22:39 UTC
(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?
Comment 72 David Cook 2023-10-15 23:18:36 UTC
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.
Comment 73 Jonathan Druart 2023-10-18 13:00:48 UTC
(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
Comment 74 David Cook 2023-11-29 02:45:36 UTC
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.
Comment 75 Jonathan Druart 2023-12-06 08:46:10 UTC
It's now or never for me.
Comment 76 David Cook 2023-12-06 23:01:56 UTC
(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.
Comment 77 Marcel de Rooy 2023-12-07 06:51:39 UTC
I am supporting this too. Just have been waiting for resulting patches from the brainstorm session :)
Comment 78 Jonathan Druart 2023-12-07 07:16:52 UTC
(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.
Comment 79 David Cook 2023-12-08 03:55:19 UTC
(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.
Comment 80 Jonathan Druart 2023-12-13 07:57:28 UTC
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.
Comment 81 Jonathan Druart 2023-12-13 08:29:48 UTC
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.
Comment 82 Martin Renvoize 2023-12-13 10:49:59 UTC
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.
Comment 83 Jonathan Druart 2023-12-13 15:56:43 UTC
Next step should be to continue the list on this pad (convert GET to POST):

https://annuel.framapad.org/p/koha_34478_get_forms
Comment 84 David Cook 2023-12-15 02:12:29 UTC
(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?
Comment 85 Jonathan Druart 2023-12-15 04:51:35 UTC
(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.
Comment 86 David Cook 2023-12-15 05:48:13 UTC
(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.)
Comment 87 Jonathan Druart 2024-01-23 16:07:50 UTC
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.
Comment 88 Jonathan Druart 2024-01-24 10:33:46 UTC
Be careful with the commit "Bug 34478: op =~ ^cud- in pl/pm".

It certainly generated some incorrect changes!
Comment 89 Jonathan Druart 2024-01-24 11:16:10 UTC
(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.
Comment 90 Jonathan Druart 2024-01-26 08:56:44 UTC
4. Rename $action (and other "wrongly" named variables) with $op

See https://annuel.framapad.org/p/koha_34478_rename_with_op
Comment 91 Jonathan Druart 2024-01-26 09:03:06 UTC
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
Comment 92 Jonathan Druart 2024-01-30 10:40:26 UTC Comment hidden (obsolete)
Comment 93 Jonathan Druart 2024-01-30 10:40:52 UTC
Remote branche rebased against master. Already a lot of conflicts and new forms added! We really need to speed up!
Comment 94 Jonathan Druart 2024-01-31 10:24:32 UTC
(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
Comment 95 Jonathan Druart 2024-02-01 08:54:48 UTC
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.
Comment 96 David Cook 2024-02-01 22:35:08 UTC Comment hidden (obsolete)
Comment 97 David Cook 2024-02-01 23:05:47 UTC Comment hidden (obsolete)
Comment 98 David Cook 2024-02-02 02:55:24 UTC
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.
Comment 99 Jonathan Druart 2024-02-02 05:49:53 UTC
(In reply to David Cook from comment #98)
> It looks like I don't have push privileges. Can I get those?

Try again now.
Comment 100 Katrin Fischer 2024-02-02 06:43:24 UTC
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.
Comment 101 Marcel de Rooy 2024-02-02 07:20:36 UTC
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?
Comment 102 Marcel de Rooy 2024-02-02 09:42:21 UTC
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?
Comment 103 Marcel de Rooy 2024-02-02 09:43:30 UTC
Probably we have similar issue for svc or REST API ?
Comment 104 Kyle M Hall 2024-02-02 14:28:47 UTC
(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.
Comment 105 David Cook 2024-02-05 00:24:23 UTC
(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?)
Comment 106 David Cook 2024-02-05 00:28:19 UTC
(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...
Comment 107 David Cook 2024-02-05 03:16:58 UTC
(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.
Comment 108 David Cook 2024-02-05 03:43:55 UTC
(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.
Comment 109 David Cook 2024-02-05 03:44:44 UTC
(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...
Comment 110 Jonathan Druart 2024-02-06 09:03:40 UTC
(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.
Comment 111 Marcel de Rooy 2024-02-06 12:06:44 UTC
(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 ;)
Comment 112 Marcel de Rooy 2024-02-06 12:45:11 UTC
(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??
Comment 113 Jonathan Druart 2024-02-06 14:58:03 UTC
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.
Comment 114 Jonathan Druart 2024-02-06 14:59:05 UTC
(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.
Comment 115 Jonathan Druart 2024-02-07 08:18:47 UTC
I've pushed xt/find-missing-op-in-forms.t, it has 88 failures right now.
Comment 116 David Cook 2024-02-09 00:00:26 UTC
(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?
Comment 117 Nick Clemens 2024-02-09 12:26:42 UTC
(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
Comment 118 Jonathan Druart 2024-02-13 08:23:10 UTC
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?
Comment 119 Jonathan Druart 2024-02-13 08:25:43 UTC
Could we allow some "op" for POST? Like "search"?..
Comment 120 Martin Renvoize 2024-02-13 11:56:18 UTC
*** Bug 32365 has been marked as a duplicate of this bug. ***
Comment 121 Martin Renvoize 2024-02-13 11:56:28 UTC
*** Bug 32364 has been marked as a duplicate of this bug. ***
Comment 122 Martin Renvoize 2024-02-13 11:56:36 UTC
*** Bug 32363 has been marked as a duplicate of this bug. ***
Comment 123 Martin Renvoize 2024-02-13 11:56:44 UTC
*** Bug 32362 has been marked as a duplicate of this bug. ***
Comment 124 Martin Renvoize 2024-02-13 11:56:51 UTC
*** Bug 32361 has been marked as a duplicate of this bug. ***
Comment 125 Martin Renvoize 2024-02-13 11:57:00 UTC
*** Bug 32360 has been marked as a duplicate of this bug. ***
Comment 126 Martin Renvoize 2024-02-13 11:57:09 UTC
*** Bug 32359 has been marked as a duplicate of this bug. ***
Comment 127 Marcel de Rooy 2024-02-13 12:28:39 UTC
(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?
Comment 128 Jonathan Druart 2024-02-13 14:29:46 UTC
(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.
Comment 129 Jonathan Druart 2024-02-13 14:30:14 UTC
Added bug 36084 - Pass CSRF token to SVC scripts

For the svc/ajax scripts part. For discussion, waiting for feedback.
Comment 130 Jonathan Druart 2024-02-13 15:20:22 UTC
(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.
Comment 131 Marcel de Rooy 2024-02-13 15:25:15 UTC
(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..
Comment 132 Jonathan Druart 2024-02-13 15:33:25 UTC Comment hidden (obsolete)
Comment 133 Kyle M Hall 2024-02-13 20:02:09 UTC
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 ).
Comment 134 David Cook 2024-02-14 00:05:26 UTC
(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.
Comment 135 David Cook 2024-02-14 00:14:14 UTC
(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.
Comment 136 Martin Renvoize 2024-02-14 08:28:51 UTC
(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?!
Comment 137 Jonathan Druart 2024-02-14 08:38:59 UTC
(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.
Comment 138 Jonathan Druart 2024-02-14 08:40:22 UTC
(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 ;)
Comment 139 Nick Clemens 2024-02-14 11:39:05 UTC
(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
Comment 140 Jonathan Druart 2024-02-14 11:47:31 UTC
(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.
Comment 141 Jonathan Druart 2024-02-14 12:37:52 UTC
Remote branch rebased on top of the dependent bugs.
Comment 142 Owen Leonard 2024-02-14 18:57:40 UTC
Created attachment 162165 [details] [review]
Bug 34478: (follow-up) Change invoice files delete link to POST
Comment 143 Jonathan Druart 2024-02-15 07:14:58 UTC
Comment on attachment 162165 [details] [review]
Bug 34478: (follow-up) Change invoice files delete link to POST

Added to the remote branch.
Comment 144 Jonathan Druart 2024-02-15 08:04:21 UTC
(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)?
Comment 145 Marcel de Rooy 2024-02-15 08:38:00 UTC
(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 ?
Comment 146 David Cook 2024-02-15 22:43:19 UTC
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.
Comment 147 Jonathan Druart 2024-02-16 07:39:29 UTC
(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.
Comment 148 Jonathan Druart 2024-02-16 07:40:14 UTC
(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.
Comment 149 David Cook 2024-02-21 00:34:03 UTC
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?
Comment 150 Jonathan Druart 2024-02-21 07:44:53 UTC
(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.
Comment 151 Jonathan Druart 2024-02-21 07:51:32 UTC
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).
Comment 152 Jonathan Druart 2024-02-21 07:59:01 UTC
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
Comment 153 Jonathan Druart 2024-02-21 09:42:30 UTC
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.
Comment 154 Jonathan Druart 2024-02-21 14:07:47 UTC
(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)
Comment 155 David Cook 2024-02-22 04:45:37 UTC
(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"
Comment 156 David Cook 2024-02-22 04:50:07 UTC
(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.
Comment 157 David Cook 2024-02-22 05:18:17 UTC
(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.
Comment 158 Jonathan Druart 2024-02-22 09:50:53 UTC
(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.
Comment 159 Jonathan Druart 2024-02-22 09:52:09 UTC
(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!)
Comment 160 Jonathan Druart 2024-02-22 09:52:46 UTC
(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.
Comment 161 Marcel de Rooy 2024-02-22 10:30:38 UTC
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 + '" />' );
+        }
+    });
Comment 162 Jonathan Druart 2024-02-22 13:12:41 UTC
(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.
Comment 163 Jonathan Druart 2024-02-22 13:28:26 UTC
(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.
Comment 164 Jonathan Druart 2024-02-22 14:53:06 UTC
Remote branch cleaned. I removed the "DO NOT PUSH" and squashed the "TO SQUASH".
Comment 165 Jonathan Druart 2024-02-22 14:53:57 UTC
(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
Comment 166 David Cook 2024-02-27 05:41:14 UTC
(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.
Comment 167 David Cook 2024-02-27 05:41:34 UTC
(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?
Comment 168 David Cook 2024-02-27 05:55:41 UTC
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.
Comment 169 David Cook 2024-02-27 06:01:03 UTC
(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);
Comment 170 David Cook 2024-02-27 06:02:03 UTC
(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
Comment 171 David Cook 2024-02-27 06:09:56 UTC
(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
Comment 172 Jonathan Druart 2024-02-27 07:44:15 UTC
(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).
Comment 173 Jonathan Druart 2024-02-27 07:46:32 UTC
> (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.
Comment 174 Jonathan Druart 2024-02-28 09:54:49 UTC
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.
Comment 175 Marcel de Rooy 2024-02-28 12:08:31 UTC
(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.
Comment 176 Jonathan Druart 2024-02-28 12:17:57 UTC
(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.
Comment 177 David Cook 2024-02-29 22:54:30 UTC
(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).
Comment 178 Marcel de Rooy 2024-03-01 07:32:43 UTC
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.
Comment 179 Marcel de Rooy 2024-03-01 07:33:49 UTC
(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 180 Jonathan Druart 2024-03-01 08:32:18 UTC
Comment on attachment 162630 [details] [review]
Bug 34478: (follow-up) Move resend link out of form for display reasons

Picked for today's push.
Comment 181 Jonathan Druart 2024-03-01 13:20:45 UTC
Pushed to master for 24.05.00.
Comment 182 Fridolin Somers 2024-03-11 08:45:43 UTC
Not for backport I bet ?
Comment 183 David Cook 2024-03-11 23:29:00 UTC
(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?
Comment 184 David Cook 2024-03-12 00:42:50 UTC
(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.
Comment 185 David Cook 2024-03-12 01:22:10 UTC
(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...
Comment 186 Fridolin Somers 2024-03-12 08:10:58 UTC
OK thanks for your comments.
For the moment no backport
Comment 187 Jonathan Druart 2024-03-14 14:49:45 UTC
*** Bug 35677 has been marked as a duplicate of this bug. ***
Comment 188 Jonathan Druart 2024-03-14 14:51:19 UTC
*** Bug 19645 has been marked as a duplicate of this bug. ***
Comment 189 Jonathan Druart 2024-03-14 14:51:25 UTC
*** Bug 22314 has been marked as a duplicate of this bug. ***
Comment 190 Jonathan Druart 2024-03-14 14:51:38 UTC
*** Bug 23060 has been marked as a duplicate of this bug. ***
Comment 191 Jonathan Druart 2024-03-14 14:51:45 UTC
*** Bug 34111 has been marked as a duplicate of this bug. ***
Comment 192 Jonathan Druart 2024-03-14 14:51:50 UTC
*** Bug 23238 has been marked as a duplicate of this bug. ***
Comment 193 Jonathan Druart 2024-03-14 14:52:18 UTC
*** Bug 30502 has been marked as a duplicate of this bug. ***
Comment 194 David Cook 2024-03-27 05:07:30 UTC
(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.