Bug 34369

Summary: Add CSRF protection to system preferences
Product: Koha Reporter: David Cook <dcook>
Component: System AdministrationAssignee: David Cook <dcook>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: 1joynelson, dcook, didier.gautheron, fridolin.somers, gmcharlt, jonathan.druart, laurent.ducos, lucas, m.de.rooy, nick, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33289
GIT URL: Change sponsored?: ---
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.04,22.11.10,22.05.16,21.11.24
Circulation function:
Bug Depends on: 30524    
Bug Blocks:    
Attachments: Bug 34369: Require CSRF token for updating system preferences
Bug 34369: Pass csrf token from syspref API client (for Vue app)
Bug 34369: Require CSRF token for updating system preferences
Bug 34369: Pass csrf token from syspref API client (for Vue app)
Bug 34369: Fix 'Did you mean'
Bug 34369: Require CSRF token for updating system preferences
Bug 34369: Pass csrf token from syspref API client (for Vue app)
Bug 34369: Fix 'Did you mean'

Description David Cook 2023-07-25 05:23:20 UTC
The system preferences need urgent CSRF protection to prevent external XSS injection and other undesirable consequences.
Comment 1 David Cook 2023-08-01 03:55:35 UTC
This one is a bit harder to exploit but I've done it previously...
Comment 4 David Cook 2023-08-01 05:53:39 UTC
It looks like preferenes.pl $op save doesn't get used anymore as an old school API is used instead and systempreferences.pl is used for "Local use preferences"...

So we'll need to write a token out to preferences.pl, find it and send it in koha-tmpl/intranet-tmpl/prog/js/pages/preferences.js, and choke without it in ./svc/config/systempreferences
Comment 6 David Cook 2023-08-01 06:43:41 UTC
Bug 33289 is going to be a problem... since it doesn't have ready access to a CSRF token...
Comment 7 David Cook 2023-08-01 06:52:15 UTC
(In reply to David Cook from comment #6)
> Bug 33289 is going to be a problem... since it doesn't have ready access to
> a CSRF token...

I actually bumped into this same problem with a Vue app I created on Wordpress that uses the Wordpress API: https://developer.wordpress.org/rest-api/using-the-rest-api/authentication/

I think the official way and the way I ended up doing was to use the server-side to print out a data structure containing the CSRF token in an inline Javascript element.

Of course, that's going to be easier said than done in Koha.

I imagine Jonathan will have more insight into handle CSRF for Bug 33289...
Comment 8 David Cook 2023-08-01 06:58:06 UTC
Created attachment 154121 [details] [review]
Bug 34369: Require CSRF token for updating system preferences

This patch adds the requirements that updating a system preference
requires a CSRF token. (Also, adding and deleting local system preferences.)

0. Apply patch
1. koha-plack --reload kohadev
2. Add local system preference
3. Update local system preference
4. Delete local system preference
5. Update normal system preference
6. Note no errors
Comment 9 Jonathan Druart 2023-08-01 07:39:39 UTC
Created attachment 154122 [details] [review]
Bug 34369: Pass csrf token from syspref API client (for Vue app)
Comment 10 Jonathan Druart 2023-08-01 07:41:10 UTC
(In reply to Jonathan Druart from comment #9)
> Created attachment 154122 [details] [review] [review]
> Bug 34369: Pass csrf token from syspref API client (for Vue app)

This is not used for now (removed from bug 33606) but will be by bug 30708 (set sysprefs from the Vue app).
Comment 11 David Cook 2023-08-02 01:05:53 UTC
Thanks, Jonathan. I really appreciate that.
Comment 12 Jonathan Druart 2023-08-03 08:04:00 UTC
Created attachment 154163 [details] [review]
Bug 34369: Require CSRF token for updating system preferences

This patch adds the requirements that updating a system preference
requires a CSRF token. (Also, adding and deleting local system preferences.)

0. Apply patch
1. koha-plack --reload kohadev
2. Add local system preference
3. Update local system preference
4. Delete local system preference
5. Update normal system preference
6. Note no errors

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2023-08-03 08:04:03 UTC
Created attachment 154164 [details] [review]
Bug 34369: Pass csrf token from syspref API client (for Vue app)

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Jonathan Druart 2023-08-03 08:04:07 UTC
Created attachment 154165 [details] [review]
Bug 34369: Fix 'Did you mean'

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Marcel de Rooy 2023-08-04 06:33:22 UTC
Looking here now
Comment 16 Marcel de Rooy 2023-08-04 07:03:39 UTC
Created attachment 154237 [details] [review]
Bug 34369: Require CSRF token for updating system preferences

This patch adds the requirements that updating a system preference
requires a CSRF token. (Also, adding and deleting local system preferences.)

0. Apply patch
1. koha-plack --reload kohadev
2. Add local system preference
3. Update local system preference
4. Delete local system preference
5. Update normal system preference
6. Note no errors

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2023-08-04 07:03:42 UTC
Created attachment 154238 [details] [review]
Bug 34369: Pass csrf token from syspref API client (for Vue app)

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2023-08-04 07:03:44 UTC
Created attachment 154239 [details] [review]
Bug 34369: Fix 'Did you mean'

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 David Cook 2023-08-30 00:50:17 UTC
In terms of CSRF fixes, this one should be a priority I think, since it has a large blast radius.
Comment 20 Tomás Cohen Arazi (tcohen) 2023-09-25 22:08:33 UTC
Note: I had to rebase it. If the dependencies are not backported, you might need to pick the patches from this report.

Pushed to master for 23.11.

Nice work everyone, thanks!