Let me explain

1. I am generating CSRF token for all the pages using Auth.pm

use Koha::Token;
$template->param( csrf_token => Koha::Token->new->generate_csrf({ session_id => scalar $sessionID }));

2. Created new csrf-token.inc include file.

Including [% INCLUDE 'csrf-token.inc' %] where form is submitting
 [% INCLUDE 'blocking_errors.inc' %] for displaying error message "WRONG CSRF" in .tt file.

3. Checking CSRF token from Output.pm
use Koha::Token;
output_and_exit_if_error

4. Calling check_csrf_token subroutine in .pl page where form submission is calling.
output_and_exit_if_error 

If this is patch we can call 

output_and_exit_if_error in every .pl page 
and
[% INCLUDE 'csrf-token.inc' %] & [% INCLUDE 'blocking_errors.inc' %] in every .tt page

Please review the patch and provide your feedback.

Let me explain

1. I am generating CSRF token for all the pages using Auth.pm

use Koha::Token;
$template->param( csrf_token => Koha::Token->new->generate_csrf({ session_id => scalar $sessionID }));

2. Created new csrf-token.inc include file.

Including [% INCLUDE 'csrf-token.inc' %] where form is submitting
 [% INCLUDE 'blocking_errors.inc' %] for displaying error message "WRONG CSRF" in .tt file.

3. Checking CSRF token from Output.pm
use Koha::Token;
output_and_exit_if_error

4. Calling check_csrf_token subroutine in .pl page where form submission is calling.
output_and_exit_if_error 

If this is patch OK we can call 

output_and_exit_if_error in every .pl page 
and
[% INCLUDE 'csrf-token.inc' %] & [% INCLUDE 'blocking_errors.inc' %] in every .tt page

Please review the patch and provide your feedback.

Let me explain

1. I am generating CSRF token for all the pages using Auth.pm

use Koha::Token;
$template->param( csrf_token => Koha::Token->new->generate_csrf({ session_id => scalar $sessionID }));

2. Created new csrf-token.inc include file.

Including [% INCLUDE 'csrf-token.inc' %] where form is submitting
 [% INCLUDE 'blocking_errors.inc' %] for displaying error message "WRONG CSRF" in .tt file.

3. Checking CSRF token from Output.pm
use Koha::Token;
output_and_exit_if_error

4. Calling "output_and_exit_if_error" subroutine in .pl page where form submission is calling.

If this is patch OK we can call 

output_and_exit_if_error in every .pl page 
and
[% INCLUDE 'csrf-token.inc' %] & [% INCLUDE 'blocking_errors.inc' %] in every .tt page

Please review the patch and provide your feedback.

Hi Amit,

1. What about performance impact?
2. Double check the changes in C4::Output, you do not need the new indentation level you added. Also, module should be "__all__" or something more meaningful than 1
It should call output_and_exit only once, at the end of the sub.
3. In basketheader.pl, second parameter should be $cookie
4. I am not sure blocking_errors.inc is included at the right place. Must be in the "main"

I did not test the patch so I may be wrong somewhere.

Also, could we imaging adding the csrf_token in JS? It will ensure the node exist in all of our form.
I am not sure it's a good idea, just a thought :)

(In reply to Jonathan Druart from comment #5)
> Hi Amit,
> 
> 1. What about performance impact?
> 2. Double check the changes in C4::Output, you do not need the new
> indentation level you added. Also, module should be "__all__" or something
> more meaningful than 1
> It should call output_and_exit only once, at the end of the sub.
> 3. In basketheader.pl, second parameter should be $cookie
> 4. I am not sure blocking_errors.inc is included at the right place. Must be
> in the "main"
> 
> I did not test the patch so I may be wrong somewhere.


Hi Jonathan,

Thanks for feedback

1. I have checked and not found any impact on performance.
2. I think we will use a module name like "CSRF_TOKEN_FIXES".
   let me check and explore the possibility if we can call "output_and_exit" 
   only once.
3. If we are passing $cookie variable - I have checked in the log it is giving: 
   CGISESSID=cecd42cecdb64e5d7b52d031f6c322e4; path=/; HttpOnly
   In this case, the token is not matching and gives the error message 

   In case if we are passing $input->cookie('CGISESSID') it is giving value: 
   cecd42cecdb64e5d7b52d031f6c322e4 and work perfectly.
4. Let me include "blocking_errors.inc" in the main page or before form.

From my point of view, JS is not a good idea to include. 

Can you please apply the patch and test.

(In reply to Amit Gupta from comment #7)
> (In reply to Jonathan Druart from comment #5)
> > Hi Amit,
> > 
> > 1. What about performance impact?
> > 2. Double check the changes in C4::Output, you do not need the new
> > indentation level you added. Also, module should be "__all__" or something
> > more meaningful than 1
> > It should call output_and_exit only once, at the end of the sub.
> > 3. In basketheader.pl, second parameter should be $cookie
> > 4. I am not sure blocking_errors.inc is included at the right place. Must be
> > in the "main"
> > 
> > I did not test the patch so I may be wrong somewhere.
> 
> 
> Hi Jonathan,
> 
> Thanks for feedback
> 
> 1. I have checked and not found any impact on performance.
> 2. I think we will use a module name like "CSRF_TOKEN_FIXES".
>    let me check and explore the possibility if we can call "output_and_exit" 
>    only once.
> 3. If we are passing $cookie variable - I have checked in the log it is
> giving: 
>    CGISESSID=cecd42cecdb64e5d7b52d031f6c322e4; path=/; HttpOnly
>    In this case, the token is not matching and gives the error message 
> 
>    In case if we are passing $input->cookie('CGISESSID') it is giving value: 
>    cecd42cecdb64e5d7b52d031f6c322e4 and work perfectly.
> 4. Let me include "blocking_errors.inc" in the main page or before form.
> 
> From my point of view, JS is not a good idea to include. 
> 
> Can you please apply the patch and test.

Hi Jonathan,

I did changes suggested by you. Can you please look into and give us your feedback.

Note I have given the module name for ex: "CSRF_TOKEN_CHECK"

(In reply to Jonathan Druart from comment #10)
> Created attachment 90194 [details] [review] [review]

This is what I had in mind.

(In reply to Amit Gupta from comment #9)
> Note I have given the module name for ex: "CSRF_TOKEN_CHECK"

It's not a "module". $module should be a Koha module, 'members', 'acquisition', etc.
Maybe it's not appropriate, the original idea was to have a list of checks to process for a given module.

Here it's not what you want, so maybe you need to introduce a new param "check"?

Created attachment 90202 [details] [review]
Bug 22990: Introduce a new param "vulnerability" to check  csrf token.

New param name as "vulnerability" to check csrf token.

(In reply to Jonathan Druart from comment #11)
> (In reply to Jonathan Druart from comment #10)
> > Created attachment 90194 [details] [review] [review] [review]
> 
> This is what I had in mind.
> 
> (In reply to Amit Gupta from comment #9)
> > Note I have given the module name for ex: "CSRF_TOKEN_CHECK"
> 
> It's not a "module". $module should be a Koha module, 'members',
> 'acquisition', etc.
> Maybe it's not appropriate, the original idea was to have a list of checks
> to process for a given module.
> 
> Here it's not what you want, so maybe you need to introduce a new param
> "check"?

Hi Jonathan,

Thanks for the input I introduce new param name as "vulnerability".

Can you please look into and give us your feedback.

"vulnerability" seems too specific to me, why not { check => 'csrf_token' }?

(In reply to Amit Gupta from comment #15)
> Created attachment 90204 [details] [review] [review]
> Bug 22990: Introduce a new param "check" to check csrf token.
> 
> Introduce a new param "check" to check csrf token.

Hi Jonathan,

Thanks for the feedback i have changed (In reply to Jonathan Druart from comment #14)
> "vulnerability" seems too specific to me, why not { check => 'csrf_token' }?

Hi Jonathan,

Thanks for your feedback I have changed param name "vulnerability" to "check".

Can you please look into and give us your feedback. So I can check and apply all the pages related to CSRF issues.

Marcel, Can you have a look at this and give us your opinion please?

(In reply to Jonathan Druart from comment #21)
> Marcel, Can you have a look at this and give us your opinion please?

Looks good to me at first glance.

Hi Jonathan,

Can I raise bug (CSRF issues related to other pages and fix)?

(In reply to Amit Gupta from comment #23)
> Hi Jonathan,
> 
> Can I raise bug (CSRF issues related to other pages and fix)?

Yes, please do!

Introduces a nice pattern for dealing with these csrf issues and works well.

Signing off.

Hi 

I have fixed CSRF for Membership/Suggestion/Acquistions and some part of OPAC for 18.11.x

I am planning to rebase on master and send all the patches for testing by next month.

I think we should wait for the other patches to move this one forward, ie. push them all in one go

Switched status to BLOCKED until we get the other patches.

Are you still intending on working on this Amit.. I think we're all waiting with bated breath here.

(In reply to Martin Renvoize from comment #40)
> Are you still intending on working on this Amit.. I think we're all waiting
> with bated breath here.

Hi Martin,

Yes, I am working on the CSRF issues most of the pages are fixed. Was Occupied with some other task that why this task got delayed.

Will give the update soon.

Hi Jonathan,

Can you add me on bug

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24409

*** Bug 24409 has been marked as a duplicate of this bug. ***

*** Bug 24416 has been marked as a duplicate of this bug. ***

Quick update:

Hi All,

Will able to give all the possible CSRF issues (staff pages) in couple of weeks.

(In reply to Amit Gupta from comment #45)
> Quick update:
> 
> Hi All,
> 
> Will able to give all the possible CSRF issues (staff pages) in couple of
> weeks.

Amit, any chances you could submit what you did already? We could start reviewing them to make sure we are going to the good direction.

Created attachment 97814 [details] [review]
Bug 22990 Add CSRF protection to Koha pages

Hi All,

I have fixed for suggestion module and membership payment related stuff.

Please review the patch and give feedback. Once this patch is OK I will commit other modules CSRF issues and submit.

I do not think it's NSO, it's more in discussion as the whole work is not done yet.

Copying here, publicly, what I told you by email:

"""
So yes, the last patch looks correct to me. I would suggest you to
submit it on the bug report to get other opinions.

However, maybe we should move the output_and_exit_if_error statement
at the top of the script, like:

if ( $op eq 'save' or $op eq 'edit' ) {
    output_and_exit_if_error($input, $cookie, $template, { check =>
'csrf_token' });
}

That would avoid having it spread in everywhere in the controllers.
"""

(In reply to Jonathan Druart from comment #48)
> I do not think it's NSO, it's more in discussion as the whole work is not
> done yet.
> 
> Copying here, publicly, what I told you by email:
> 
> """
> So yes, the last patch looks correct to me. I would suggest you to
> submit it on the bug report to get other opinions.
> 
> However, maybe we should move the output_and_exit_if_error statement
> at the top of the script, like:
> 
> if ( $op eq 'save' or $op eq 'edit' ) {
>     output_and_exit_if_error($input, $cookie, $template, { check =>
> 'csrf_token' });
> }
> 
> That would avoid having it spread in everywhere in the controllers.
> """

Thanks, Jonathan

Once we will receive the feedback. I will commit all the .pl and .tt files and make one big patch for signoff.

(In reply to Amit Gupta from comment #49)
> (In reply to Jonathan Druart from comment #48)
> > I do not think it's NSO, it's more in discussion as the whole work is not
> > done yet.
> > 
> > Copying here, publicly, what I told you by email:
> > 
> > """
> > So yes, the last patch looks correct to me. I would suggest you to
> > submit it on the bug report to get other opinions.
> > 
> > However, maybe we should move the output_and_exit_if_error statement
> > at the top of the script, like:
> > 
> > if ( $op eq 'save' or $op eq 'edit' ) {
> >     output_and_exit_if_error($input, $cookie, $template, { check =>
> > 'csrf_token' });
> > }
> > 
> > That would avoid having it spread in everywhere in the controllers.
> > """
> 
> Thanks, Jonathan
> 
> Once we will receive the feedback. I will commit all the .pl and .tt files
> and make one big patch for signoff.

While fixing CSRF I have seen in some .pl pages we are calling 

"add_validate" so it will take care both save and edit.

for ex: cities.pl page
-------------------------------------------------------------
elsif ( $op eq 'add_validate' ) {
    my $cityid       = $input->param('cityid');
    my $city_name    = $input->param('city_name');
    my $city_state   = $input->param('city_state');
    my $city_zipcode = $input->param('city_zipcode');
    my $city_country = $input->param('city_country');

--------------------------------------------------------------

Hi All,

Can you give us feedback on the last patch?

No answer means everybody agreed, right?

> Params "module" and "check" are mutually exclusive.

Hmm.. why do 'check' and 'module' need to be mutually exclusive?

Looks like koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt is missing the include of a blocking_errors block.

Amit, are you still planning to work on this?

(In reply to Jonathan Druart from comment #55)
> Amit, are you still planning to work on this?

Hi Jonathan,

Actually I have fixed for the 18.11 version but due some time constraints not able to rebase the patch on the master branch. I planning to rebase all the patches related to CSRF and submit soon.

How can we move this one forward now?

I've rebased what we had.. it's a shame Amit never submitted his final patch.. even if incomplete it would have given us something further along to continue on.

Adding in a comment what I have in my IRC logs (from last year).

"""
I've just found https://metacpan.org/pod/Plack::Middleware::CSRFBlock
but actually it does not work for "us"
but I have no idea how it could work for anybody else
it put the csrf token in all the forms it found
and check posted form
BUT, form can be posted as... GET 
and so it becomes plain useless...
"""

And from today with Martin:
"""
So maybe not worth spending more time
then go with 22990
I think they all need to be moved to 22990
22314 22315 23060 
we don't need dozen of bugs
just "fix all them all at once" big bug
"""

(In reply to Martin Renvoize from comment #65)
> I've rebased what we had.. it's a shame Amit never submitted his final
> patch.. even if incomplete it would have given us something further along to
> continue on.

Martin,

Last time I have fixed the issue through AWS WAF settings for the client. 

I have fixed CSRF issues most of the pages for 18.11 but it's require lot of time for testing before submitting to the community.

Right.. that final patch gives us an easier route to identifying the missing cases.. though currently it only works for forms (and excludes 'get' method forms of which we have many which I believe are errors in themselves).

It does not help us with the plain link type submissions, which also need some handling at some point.

QA Script update also submitted.

Using the xt script I added there are still 360 instances that need some attention even after these patches are applied.. 

Could I get some help here to work through those.. or does anyone have a wider catch approach that would require less maintenance work?

I didn't realize that Martin had added some ready patches here!

I've been looking at these again, and it's looking good to me at the core.

+1 to generating a csrf token via C4::Auth for every request (I've been doing this on a custom Koha and it makes life easier than doing it on a per-page basis...)
+1 for having a csrf-token.inc include to make it easier to add to POST forms
+1 for using "output_and_exit_if_error($input, $cookie, $template, { check => 'csrf_token' });" 

It would be nice if there were an easier way...

On one hand, the SameSite=Lax cookie attribute from Bug 26019 should help us for POST requests (https://owasp.org/www-community/SameSite), although it won't for GET requests (and we're vulnerable to GETs being used for things that should be POSTed...)

I've looked a little at other resources and it seems that they take advantage of things like form builders to add automatic CSRF protection although even then it would just be for forms and not GETs via links...

I was also thinking... we allow cookie authentication for the REST API, so the REST API is also vulnerable to CSRF in theory. I haven't checked for sure, but I can't recall any CSRF protection for it.

I recall DSpace use a CSRF token with their new REST API. I think it's a bit cumbersome: https://wiki.lyrasis.org/display/DSPACE/REST+Authentication

I haven't looked in depth at how Keycloak does it (which implements its UI using a front-end framework): https://github.com/keycloak/keycloak-documentation/blob/main/server_admin/topics/threat/csrf.adoc

I suppose we could implement our own "form builder" and "link builder" that would automatically add csrf tokens... although I think that would be a huge refactoring effort. 

Plus, at the end of the day, we don't want to have CSRF token verification on bookmarkable/shareable links...

I have considered that we figure out a model for creating new pages that are easily secured and then use that for all new content, and slowly refactor existing pages into using that model. Easier said than done for sure though.

Some OPAC actions that should have CSRF protection:
- Adding new private lists
- Deleting private lists
- Deleting book suggestion requests
- Reserving items
- Deleting reserved items

(In reply to Martin Renvoize from comment #89)
> Using the xt script I added there are still 360 instances that need some
> attention even after these patches are applied.. 
> 
> Could I get some help here to work through those.. or does anyone have a
> wider catch approach that would require less maintenance work?

We already have some CSRF handling in our code, so I don't know that we have to do everything all at once either. Maybe better to get the core mechanism in place via 1 bug report (ie this one), and then have follow-up bug reports to handle the rest. 

That would help with backporting as well I imagine...

Looking back over these patches... they really would benefit from a squash.

Actually, I don't think this implementation of output_and_exit_if_error is great, as it makes it quite labour intensive to just add CSRF protection to even 1 page. 

Better to just generate a 500 Internal Server Error I think.

Anyway, I hope to have covered off the majority of the OPAC by early next week.

So if we can agree on a method I would happy to submit that work.

(In reply to David Cook from comment #109)
> Actually, I don't think this implementation of output_and_exit_if_error is
> great, as it makes it quite labour intensive to just add CSRF protection to
> even 1 page. 
> 
> Better to just generate a 500 Internal Server Error I think.

(Almost) Never 500, it's a goal. I don't understand what is "labour intensive" with output_and_exit_if_error. You just need to call it and that's all.

(In reply to David Cook from comment #110)
> Anyway, I hope to have covered off the majority of the OPAC by early next
> week.
> 
> So if we can agree on a method I would happy to submit that work.

Same method as what we are doing for years? :)
Or a generic method, if you have something to suggest.

(In reply to Jonathan Druart from comment #111)
> (Almost) Never 500, it's a goal. I don't understand what is "labour
> intensive" with output_and_exit_if_error. You just need to call it and
> that's all.
> 

It looks like I misunderstood the role of 'blocking_errors.inc'. My bad! That's a very interesting include. 

> (In reply to David Cook from comment #110)
> > Anyway, I hope to have covered off the majority of the OPAC by early next
> > week.
> > 
> > So if we can agree on a method I would happy to submit that work.
> 
> Same method as what we are doing for years? :)

The method we've been using for years is slow and painful (for developers) in comparison to what is attached to this bug report though. 

> Or a generic method, if you have something to suggest.

I think the method used here is good. Before I changed to "Patch doesn't apply", it was marked "In Discussion" though, but I don't know what's left to discuss?

Could we rebase and squash this down to just include the changes to C4/Auth.pm and C4/Output.pm and then have a follow-up report for changes to Perl scripts? 

Since I'm currently working on this, I'd be keen to share my work on the OPAC side without having to depend on changes to the Staff Interface side before being pushed (since updating the Staff Interface will be much slower than the OPAC).

*** Bug 17096 has been marked as a duplicate of this bug. ***

Right.. rebased, then squashed and split into two bugs.. one for the core changes another for the uses done so far..

I've been chasing this round since mid 2019.. could someone just actually test it and sign off on it now please.. I can't keep working like this and it's a security issue folks!

(In reply to Martin Renvoize from comment #131)
> Right.. rebased, then squashed and split into two bugs.. one for the core
> changes another for the uses done so far..
> 

Much appreciated!

> I've been chasing this round since mid 2019.. could someone just actually
> test it and sign off on it now please.. I can't keep working like this and
> it's a security issue folks!

I might not get to it this week, as I'm only here for 2 days before I'm back on holidays, but I'm going to make this a priority for next week at least.

Test plan:
0. Apply patch
1. koha-plack --restart kohadev
2. http://localhost:8081/cgi-bin/koha/acqui/basketheader.pl?op=add_validate&booksellerid=1&basketname=attack

Bumped into some code errors, so marking as Failed QA. I'll provide a follow-up patch and see how far I get...

Test plan:
0. Apply patch
1. koha-plack --restart kohadev

2. http://localhost:8081/cgi-bin/koha/acqui/basketheader.pl?op=add_validate&booksellerid=1&basketname=attack
2b. CSRF prevention activated

3. http://localhost:8081/cgi-bin/koha/members/boraccount.pl?action=void&borrowernumber=51
3b. http://localhost:8081/cgi-bin/koha/members/boraccount.pl?action=payout&borrowernumber=51
3c. http://localhost:8081/cgi-bin/koha/members/boraccount.pl?action=refund&borrowernumber=51
3d. http://localhost:8081/cgi-bin/koha/members/boraccount.pl?action=discount&borrowernumber=51
3e. CSRF prevention activated for all

4. http://localhost:8081/cgi-bin/koha/members/pay.pl?borrowernumber=51&paycollect=1
4b. http://localhost:8081/cgi-bin/koha/members/pay.pl?borrowernumber=51&payselected=1
4c. http://localhost:8081/cgi-bin/koha/members/pay.pl?borrowernumber=51&writeoff_selected=1
4d. http://localhost:8081/cgi-bin/koha/members/pay.pl?borrowernumber=51&woall=1
4e. http://localhost:8081/cgi-bin/koha/members/pay.pl?borrowernumber=51&apply_credits=1
4f. http://localhost:8081/cgi-bin/koha/members/pay.pl?borrowernumber=51&confirm_writeoff=1
4g. CSRF prevention activated for all

5. http://localhost:8081/cgi-bin/koha/suggestion/suggestion.pl?op=save
5b. http://localhost:8081/cgi-bin/koha/suggestion/suggestion.pl?op=edit
5c. http://localhost:8081/cgi-bin/koha/suggestion/suggestion.pl?op=update_status
5d. http://localhost:8081/cgi-bin/koha/suggestion/suggestion.pl?op=delete

6. http://localhost:8081/cgi-bin/koha/members/setstatus.pl?borrowernumber=51&reregistration=y
6b. CSRF prevention activated (500 error)

7. http://localhost:8081/cgi-bin/koha/circ/add_message.pl
7b. No error message is displayed but CSRF is prevented

8. http://localhost:8081/cgi-bin/koha/virtualshelves/shelves.pl?op=edit_form
8b. http://localhost:8081/cgi-bin/koha/virtualshelves/shelves.pl?op=add
8c. http://localhost:8081/cgi-bin/koha/virtualshelves/shelves.pl?op=edit
8d. http://localhost:8081/cgi-bin/koha/virtualshelves/shelves.pl?op=delete
8e. http://localhost:8081/cgi-bin/koha/virtualshelves/shelves.pl?op=add_biblio
8f. http://localhost:8081/cgi-bin/koha/virtualshelves/shelves.pl?op=remove_biblios
8g. CSRF prevention activated for all

Created attachment 133422 [details] [review]
Bug 22990: Add CSRF protection to basketheader.pl

Test plan
1. Create vendor
2. Create basket using the form

<html>
<body>
<form method="POST" action="http://ip:8080/cgi-bin/koha/acqui/basketheader.pl?booksellerid=1&op=add_form">
<input type="hidden" name="booksellerid" value="1"/>
<input type="hidden" name="basketname" value="CSRFAttack-Test"/>
<input type="hidden" name="billingplace" value="MPL"/>
<input type="hidden" name="deliveryplace" value="MPL"/>
<input type="hidden" name="basketbooksellerid" value="1"/>
<input type="hidden" name="basketnote" value=""/>
<input type="hidden" name="basketbooksellernote" value=""/>
<input type="hidden" name="create_items" value="receiving"/>
<input type="hidden" name="op" value="add_validate"/>
<input type="submit" value="Submit">
</form>
</body>
<html>
3. You can able to create the basket.
4. Apply the patch.
5. Create basket using the form
6. Now you can get error "The form submission failed (Wrong CSRF token).
Try to come back, refresh the page, then try again."

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 133423 [details] [review]
Bug 22990: Add CSRF protection to Koha pages

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 133424 [details] [review]
Bug 22990: Add CSRF protection to add_message

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 133425 [details] [review]
Bug 22990: Fix for shelves table

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 133426 [details] [review]
Bug 22990: Fix for setstatus

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 133427 [details] [review]
Bug 22990: Follow-up to fix merge mistakes

Signed off your patches, Martin, and leaving "Needs Signoff" for my follow-up.

Please run the QA tool and follow-up

Created attachment 134848 [details] [review]
Bug 22990: Add CSRF protection to basketheader.pl

Test plan
1. Create vendor
2. Create basket using the form

<html>
<body>
<form method="POST" action="http://ip:8080/cgi-bin/koha/acqui/basketheader.pl?booksellerid=1&op=add_form">
<input type="hidden" name="booksellerid" value="1"/>
<input type="hidden" name="basketname" value="CSRFAttack-Test"/>
<input type="hidden" name="billingplace" value="MPL"/>
<input type="hidden" name="deliveryplace" value="MPL"/>
<input type="hidden" name="basketbooksellerid" value="1"/>
<input type="hidden" name="basketnote" value=""/>
<input type="hidden" name="basketbooksellernote" value=""/>
<input type="hidden" name="create_items" value="receiving"/>
<input type="hidden" name="op" value="add_validate"/>
<input type="submit" value="Submit">
</form>
</body>
<html>
3. You can able to create the basket.
4. Apply the patch.
5. Create basket using the form
6. Now you can get error "The form submission failed (Wrong CSRF token).
Try to come back, refresh the page, then try again."

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 134849 [details] [review]
Bug 22990: Add CSRF protection to Koha pages

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 134850 [details] [review]
Bug 22990: Add CSRF protection to add_message

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 134851 [details] [review]
Bug 22990: Fix for shelves table

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 134852 [details] [review]
Bug 22990: Fix for setstatus

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 134853 [details] [review]
Bug 22990: Follow-up to fix merge mistakes

Created attachment 134854 [details] [review]
Bug 22990: Fix template toolkit syntax issues in shelves_results.tt

(In reply to Owen Leonard from comment #142)
> Please run the QA tool and follow-up

Done!

Two things:
- The right syntax is (follow-up).
- If the bug is in NSO status, we should squash minor fixes. There's a thin line when other devs are involved, but do it as much as possible.

I haven't read the patches, just a suggestion in case it applies on this case, for now.

Marking as FQA so it gets a tidy and we move forward with this ASAP.

Created attachment 141294 [details] [review]
Bug 22990: Add CSRF protection to basketheader.pl

Test plan
1. Create vendor
2. Create basket using the form

<html>
<body>
<form method="POST" action="http://ip:8080/cgi-bin/koha/acqui/basketheader.pl?booksellerid=1&op=add_form">
<input type="hidden" name="booksellerid" value="1"/>
<input type="hidden" name="basketname" value="CSRFAttack-Test"/>
<input type="hidden" name="billingplace" value="MPL"/>
<input type="hidden" name="deliveryplace" value="MPL"/>
<input type="hidden" name="basketbooksellerid" value="1"/>
<input type="hidden" name="basketnote" value=""/>
<input type="hidden" name="basketbooksellernote" value=""/>
<input type="hidden" name="create_items" value="receiving"/>
<input type="hidden" name="op" value="add_validate"/>
<input type="submit" value="Submit">
</form>
</body>
<html>
3. You can able to create the basket.
4. Apply the patch.
5. Create basket using the form
6. Now you can get error "The form submission failed (Wrong CSRF token).
Try to come back, refresh the page, then try again."

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Cook <dcook@prosentient.com.au>

Bug 22990: Add CSRF protection to Koha pages

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 141295 [details] [review]
Bug 22990: Add CSRF protection to add_message

Signed-off-by: David Cook <dcook@prosentient.com.au>

Bug 22990: Fix for shelves table

Signed-off-by: David Cook <dcook@prosentient.com.au>

Bug 22990: Fix template toolkit syntax issues in shelves_results.tt

Bug 22990: Fix for setstatus

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 141296 [details] [review]
Bug 22990: (follow-up) fix merge mistakes

Signed-off-by: David Cook <dcook@prosentient.com.au>

Marking this as Signed Off.

It's not perfect, but it's a starting point. Perfect is the enemy of good, and we really shouldn't be delaying CSRF fixes any longer.

Created attachment 144315 [details] [review]
Bug 22990: Add CSRF protection to Koha pages

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 144316 [details] [review]
Bug 22990: Fix for shelves table

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 144317 [details] [review]
Bug 22990: Fix for setstatus

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 144318 [details] [review]
Bug 22990: Follow-up to fix merge mistakes

Created attachment 144319 [details] [review]
Bug 22990: Fix template toolkit syntax issues in shelves_results.tt

Created attachment 144320 [details] [review]
Bug 22990: Add CSRF protection to basketheader.pl

Test plan
1. Create vendor
2. Create basket using the form

<html>
<body>
<form method="POST" action="http://ip:8080/cgi-bin/koha/acqui/basketheader.pl?booksellerid=1&op=add_form">
<input type="hidden" name="booksellerid" value="1"/>
<input type="hidden" name="basketname" value="CSRFAttack-Test"/>
<input type="hidden" name="billingplace" value="MPL"/>
<input type="hidden" name="deliveryplace" value="MPL"/>
<input type="hidden" name="basketbooksellerid" value="1"/>
<input type="hidden" name="basketnote" value=""/>
<input type="hidden" name="basketbooksellernote" value=""/>
<input type="hidden" name="create_items" value="receiving"/>
<input type="hidden" name="op" value="add_validate"/>
<input type="submit" value="Submit">
</form>
</body>
<html>
3. You can able to create the basket.
4. Apply the patch.
5. Create basket using the form
6. Now you can get error "The form submission failed (Wrong CSRF token).
Try to come back, refresh the page, then try again."

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: David Cook <dcook@prosentient.com.au>

Bug 22990: Add CSRF protection to Koha pages

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 144321 [details] [review]
Bug 22990: Add CSRF protection to add_message

Signed-off-by: David Cook <dcook@prosentient.com.au>

Bug 22990: Fix for shelves table

Signed-off-by: David Cook <dcook@prosentient.com.au>

Bug 22990: Fix template toolkit syntax issues in shelves_results.tt

Bug 22990: Fix for setstatus

Signed-off-by: David Cook <dcook@prosentient.com.au>

Created attachment 144322 [details] [review]
Bug 22990: More rebase fixes

Another rebase.. but it'll need some testing again to check I got it all right...

I'm going to create a series of bugs to work through each Koha module and then try and get my team to work through them.

(In reply to Martin Renvoize from comment #164)
> Another rebase.. but it'll need some testing again to check I got it all
> right...
> 
> I'm going to create a series of bugs to work through each Koha module and
> then try and get my team to work through them.

Moving to Blocked. Since it seems to be work in progress? Please correct me if I am wrong.

I am rebasing the first patch now

 [% IF ( accounts ) %]
     <form action="/cgi-bin/koha/members/pay.pl" method="post" id="pay-fines-form">
     <input type="hidden" name="borrowernumber" id="borrowernumber" value="[% patron.borrowernumber | html %]" />
+    [% INCLUDE 'csrf-token.inc' %]
 <p><span class="checkall"><a id="CheckAll" href="#"><i class="fa fa-check"></i> Select all</a></span> | <span class="clearall"><a id="CheckNone" href="#"><i class="fa fa-remove"></i> Clear all</a></span></p>
 <table id="finest">

This looks weird. Where is the [% END %] ?

(In reply to Marcel de Rooy from comment #167)
>  [% IF ( accounts ) %]
>      <form action="/cgi-bin/koha/members/pay.pl" method="post"
> id="pay-fines-form">
>      <input type="hidden" name="borrowernumber" id="borrowernumber"
> value="[% patron.borrowernumber | html %]" />
> +    [% INCLUDE 'csrf-token.inc' %]
>  <p><span class="checkall"><a id="CheckAll" href="#"><i class="fa
> fa-check"></i> Select all</a></span> | <span class="clearall"><a
> id="CheckNone" href="#"><i class="fa fa-remove"></i> Clear all</a></span></p>
>  <table id="finest">
> 
> This looks weird. Where is the [% END %] ?

The indentation here is just a mess.

diff --git a/members/setstatus.pl b/members/setstatus.pl
index 83ef1590be..2cf0f92377 100755
--- a/members/setstatus.pl
+++ b/members/setstatus.pl
@@ -53,6 +53,7 @@ my $patron         = Koha::Patrons->find( $borrowernumber );
 if ( $logged_in_user->can_see_patron_infos($patron) ) {
     if ( $reregistration eq 'y' ) {
         # re-reregistration function to automatic calcul of date expiry
+        output_and_exit_if_error($input, $cookie, $template, { check => 'csrf_token' });
         $dateexpiry = $patron->renew_account;
     } else {
         my $sth = $dbh->prepare("UPDATE borrowers SET debarred = ?, debarredcomment = '' WHERE borrowernumber = ?");


We should move this somewhere else. How do we get a token here? We are only calling checkauth. Removing this from the first patch now.

Comment on attachment 144317 [details] [review]
Bug 22990: Fix for setstatus

Obsoleting this one too. Needs separate attention

Undefined subroutine &CGI::Compile::ROOT::usr_share_koha_suggestion_suggestion_2epl::output_and_exit_if_error called at /usr/share/koha/suggestion/suggestion.pl line 137

(In reply to Marcel de Rooy from comment #171)
> Undefined subroutine
> &CGI::Compile::ROOT::usr_share_koha_suggestion_suggestion_2epl::
> output_and_exit_if_error called at /usr/share/koha/suggestion/suggestion.pl
> line 137

Yes. I need to cherrypick from the other patches :)

Created attachment 152626 [details] [review]
Bug 22990: Add CSRF protection to boraccount, pay and suggestion

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Test plan would have been nioe.
Tested by changing MAX_AGE with suggestions.

Created attachment 152627 [details] [review]
Bug 22990: Fix for shelves table

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 152628 [details] [review]
Bug 22990: Fix template toolkit syntax issues in shelves_results.tt

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Apart from setstatus which is a weird script and needs more attention, I still have those two diffs:

diff --git a/acqui/basketheader.pl b/acqui/basketheader.pl
index e79daa70a7..90168f0d84 100755
--- a/acqui/basketheader.pl
+++ b/acqui/basketheader.pl
@@ -49,7 +49,7 @@ use Modern::Perl;
 use CGI qw ( -utf8 );
 use C4::Context;
 use C4::Auth qw( get_template_and_user );
-use C4::Output qw( output_html_with_http_headers );
+use C4::Output qw( output_html_with_http_headers output_and_exit_if_error );
 use C4::Acquisition qw( GetBasket ModBasket ModBasketHeader NewBasket );
 use C4::Contract qw( GetContracts GetContract );

diff --git a/circ/add_message.pl b/circ/add_message.pl
index 37fb2d14bc..85719a7a35 100755
--- a/circ/add_message.pl
+++ b/circ/add_message.pl
@@ -22,7 +22,7 @@ use Modern::Perl;
 use CGI qw ( -utf8 );

 use C4::Auth qw( get_template_and_user );
-use C4::Output;
+use C4::Output qw( output_and_exit_if_error );
 use Koha::Patron::Message;

 my $input = CGI->new;

(In reply to Marcel de Rooy from comment #176)
> Apart from setstatus which is a weird script and needs more attention, I
> still have those two diffs:
> 
> diff --git a/acqui/basketheader.pl b/acqui/basketheader.pl
> index e79daa70a7..90168f0d84 100755
> --- a/acqui/basketheader.pl
> +++ b/acqui/basketheader.pl
> @@ -49,7 +49,7 @@ use Modern::Perl;
>  use CGI qw ( -utf8 );
>  use C4::Context;
>  use C4::Auth qw( get_template_and_user );
> -use C4::Output qw( output_html_with_http_headers );
> +use C4::Output qw( output_html_with_http_headers output_and_exit_if_error );
>  use C4::Acquisition qw( GetBasket ModBasket ModBasketHeader NewBasket );
>  use C4::Contract qw( GetContracts GetContract );
> 
> diff --git a/circ/add_message.pl b/circ/add_message.pl
> index 37fb2d14bc..85719a7a35 100755
> --- a/circ/add_message.pl
> +++ b/circ/add_message.pl
> @@ -22,7 +22,7 @@ use Modern::Perl;
>  use CGI qw ( -utf8 );
> 
>  use C4::Auth qw( get_template_and_user );
> -use C4::Output;
> +use C4::Output qw( output_and_exit_if_error );
>  use Koha::Patron::Message;
> 
>  my $input = CGI->new;

I do not understand why I dont see template changes for them and calls to output_and_exit_if_error. So lets move these elsewhere too.
So this can go further.

Lets push this first.

Opened 34111 for the leftovers from here.

Created attachment 153394 [details] [review]
Bug 22990: (follow-up) Fix suggestion.pl

This patch doesn't apply cleanly for 21.11.x, could this please be rebased?

I've attached a rebased and squashed version for 21.11. I have not tested it though.

Created attachment 153753 [details] [review]
Bug 22990: [22.11] Add CSRF protection to boraccount, pay and suggestion

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Test plan would have been nioe.
Tested by changing MAX_AGE with suggestions.

Bug 22990: Fix for shelves table

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 22990: Fix template toolkit syntax issues in shelves_results.tt

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 22990: (follow-up) Fix suggestion.pl

(In reply to Danyon Sewell from comment #185)
> Have just tried applying again, while it's certainly cleaner this time i'm
> still getting a couple of conflicts that are a bit out of my depth, pasting
> below for reference.
 
Sometimes, rather than rebasing, it can be easier to just re-implement the patch by looking at the Splinter Review.

I could potentially look at this next week. Ran out of time this week though...

(In reply to Danyon Sewell from comment #185)
> (In reply to Jonathan Druart from comment #183)
> > I've attached a rebased and squashed version for 21.11. I have not tested it
> > though.
> 
> Hey Jonathan.
> 
> Have just tried applying again, while it's certainly cleaner this time i'm
> still getting a couple of conflicts that are a bit out of my depth, pasting
> below for reference.

Did you apply the 21.11 patches only?

It works for me:

% git checkout -b bug_22990-21.11 origin/21.11.x
% git bz apply 22990

Bug 22990 Depends on bug 30524 (Passed QA)
Follow? [(y)es, (n)o] y

Bug 30524 - Add base framework for dealing with CSRF in Koha

152635 - Bug 30524: Core CSRF checking code
152638 - Bug 30524: Unit tests
153752 - Bug 30524: [21.11] Core CSRF checking code

Apply? [(y)es, (n)o, (i)nteractive] i
Applying: Bug 30524: [21.11] Core CSRF checking code

Bug 22990 - Add CSRF protection to boraccount, pay, suggestions and virtualshelves on staff

152626 - Bug 22990: Add CSRF protection to boraccount, pay and suggestion
152627 - Bug 22990: Fix for shelves table
152628 - Bug 22990: Fix template toolkit syntax issues in shelves_results.tt
153394 - Bug 22990: (follow-up) Fix suggestion.pl
153753 - Bug 22990: [22.11] Add CSRF protection to boraccount, pay and suggestion

Apply? [(y)es, (n)o, (i)nteractive] i
Applying: Bug 22990: [22.11] Add CSRF protection to boraccount, pay and suggestion

(In reply to Danyon Sewell from comment #185)
> >>>>>>> Bug 22990: Fix for shelves table

You should not apply this patch, only:
Bug 22990: [22.11] Add CSRF protection to boraccount, pay and suggestion

Pushed to master for 23.11.

Nice work everyone, thanks!

Created attachment 154044 [details] [review]
Bug 22990: [22.05.x] Add CSRF protection to boraccount, pay and suggestion

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Test plan would have been nioe.
Tested by changing MAX_AGE with suggestions.

Bug 22990: Fix for shelves table

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 22990: Fix template toolkit syntax issues in shelves_results.tt

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 22990: (follow-up) Fix suggestion.pl

Pushed to 23.05.x for 23.05.02

*** Bug 22315 has been marked as a duplicate of this bug. ***