Bug 35955 - New CSRF token generated everytime we need one
Summary: New CSRF token generated everytime we need one
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Cook
QA Contact: Nick Clemens (kidclamp)
URL:
Keywords:
Depends on:
Blocks: 34478
  Show dependency treegraph
 
Reported: 2024-01-31 11:31 UTC by Jonathan Druart
Modified: 2024-03-12 10:56 UTC (History)
10 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,23.11.04


Attachments
Bug 35955: Cache CSRF token in template plugin (1.56 KB, patch)
2024-02-27 07:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35955: Add tests (1.71 KB, patch)
2024-02-27 07:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35955: Cache CSRF token in template plugin (1.62 KB, patch)
2024-02-27 15:25 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35955: Add tests (1.76 KB, patch)
2024-02-27 15:25 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2024-01-31 11:31:28 UTC
Why do we generate a new token everytime? I thought it was one per session.
Comment 1 David Cook 2024-01-31 22:10:28 UTC
(In reply to Jonathan Druart from comment #0)
> Why do we generate a new token everytime? I thought it was one per session.

I don't think we've ever done per-session tokens for CSRF. I think it's always been per-request, as they're regarded as more secure than per-session tokens (based on the OWASP CSRF prevention guidelines at least).
Comment 2 Marcel de Rooy 2024-02-01 07:22:07 UTC
(In reply to David Cook from comment #1)
> (In reply to Jonathan Druart from comment #0)
> > Why do we generate a new token everytime? I thought it was one per session.
> 
> I don't think we've ever done per-session tokens for CSRF. I think it's
> always been per-request, as they're regarded as more secure than per-session
> tokens (based on the OWASP CSRF prevention guidelines at least).

Synchronizer Token Pattern
CSRF tokens should be generated on the server-side and they should be generated only once per user session or each request. Because the time range for an attacker to exploit the stolen tokens is minimal for per-request tokens, they are more secure than per-session tokens. However, using per-request tokens may result in usability concerns. [Back button etc]

And since we chose for request years ago, why change it now? Good reasons?
Comment 3 Jonathan Druart 2024-02-01 07:37:58 UTC
We are not doing "per-request", we are generating one for each form. If there are several forms on the same page we will generate several ones.

I am not asking to change how we generate it, but asking if it is relevant and if this is what we really want.
Comment 4 Marcel de Rooy 2024-02-01 08:05:52 UTC
(In reply to Jonathan Druart from comment #3)
> We are not doing "per-request", we are generating one for each form. If
> there are several forms on the same page we will generate several ones.
> 
> I am not asking to change how we generate it, but asking if it is relevant
> and if this is what we really want.

Thx for clarifying :)
Comment 5 David Cook 2024-02-01 22:59:32 UTC
(In reply to Jonathan Druart from comment #3)
> We are not doing "per-request", we are generating one for each form. If
> there are several forms on the same page we will generate several ones.
> 
> I am not asking to change how we generate it, but asking if it is relevant
> and if this is what we really want.

I also appreciate the clarification.

I just took a look at Koha/Template/Plugin/Koha.pm and I see what you mean. 

No, I don't think that we really want this. I think we should generate it once, store it in the L1 cache, and re-use the same token string for the duration of the HTTP request.
Comment 6 David Cook 2024-02-01 23:00:49 UTC
(In reply to David Cook from comment #5)
> No, I don't think that we really want this. I think we should generate it
> once, store it in the L1 cache, and re-use the same token string for the
> duration of the HTTP request.

Actually we should be able to get away with putting it in the Koha/Templates/Plugin/Koha object instead of using L1 cache, since I think the lifecycle of that object is tied to the HTTP request anyway...
Comment 7 Jonathan Druart 2024-02-27 07:57:03 UTC
Created attachment 162485 [details] [review]
Bug 35955: Cache CSRF token in template plugin

This change uses the Koha::Cache::Memory::Lite cache to
cache the CSRF token, so that it is only generated once,
and is re-used by the Koha::Template::Plugin::Koha object
throughout the entire template processing for the HTTP request.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 8 Jonathan Druart 2024-02-27 07:57:07 UTC
Created attachment 162486 [details] [review]
Bug 35955: Add tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Nick Clemens (kidclamp) 2024-02-27 15:25:46 UTC
Created attachment 162511 [details] [review]
Bug 35955: Cache CSRF token in template plugin

This change uses the Koha::Cache::Memory::Lite cache to
cache the CSRF token, so that it is only generated once,
and is re-used by the Koha::Template::Plugin::Koha object
throughout the entire template processing for the HTTP request.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Nick Clemens (kidclamp) 2024-02-27 15:25:48 UTC
Created attachment 162512 [details] [review]
Bug 35955: Add tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Jonathan Druart 2024-03-01 13:20:25 UTC
Pushed to master for 24.05.00.
Comment 12 Fridolin Somers 2024-03-11 09:06:13 UTC
May we backport this for better performance ?
Comment 13 David Cook 2024-03-11 22:21:09 UTC
(In reply to Fridolin Somers from comment #12)
> May we backport this for better performance ?

I can't imagine that it would hurt.
Comment 14 Fridolin Somers 2024-03-12 08:11:54 UTC
OK thanks, will do
Comment 15 Fridolin Somers 2024-03-12 10:56:58 UTC
Pushed to 23.11.x for 23.11.04