Bug 17110

Summary: Lower CSRF expiry in Koha::Token
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Architecture, internals, and plumbingAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart+koha, kyle, mtj, veron
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17187
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 17109    
Bug Blocks: 17096    
Attachments: Bug 17110: Lower CSRF expiry in Koha::Token
Bug 17110: Add unit test for MaxAge parameter in Token.t
Bug 17110: [EXAMPLE PATCH] CSRF expiry of 10 seconds in opac-sendbasket
Bug 17110: Lower CSRF expiry in Koha::Token
Bug 17110: Add unit test for MaxAge parameter in Token.t
Bug 17110: [EXAMPLE PATCH] CSRF expiry of 10 seconds in opac-sendbasket
Bug 17110: Lower CSRF expiry in Koha::Token
Bug 17110: Add unit test for MaxAge parameter in Token.t

Description Marcel de Rooy 2016-08-11 13:25:56 UTC

    
Comment 1 Marcel de Rooy 2016-08-11 13:31:12 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2016-08-11 13:31:41 UTC
Example patch and adjusted test still coming..
Comment 3 Marcel de Rooy 2016-08-12 06:18:06 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2016-08-12 06:18:09 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2016-08-15 13:25:45 UTC Comment hidden (obsolete)
Comment 6 Marc Véron 2016-08-15 13:26:34 UTC Comment hidden (obsolete)
Comment 7 Marc Véron 2016-08-15 13:27:21 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2016-08-22 14:30:44 UTC
Could you please detail why you need this change?
I will break the following use case:
- Start to fill a form
- *Ring belt end of the day*
- you hurry up to get back at home quickly
- Tomorrow morning, you finish to fill the form
- Submit
- You lost your changes

Ok it's a bit far-fetched but I don't understand what will bring us this 8-hours limitation.
Comment 9 Marcel de Rooy 2016-08-23 12:43:08 UTC
(In reply to Jonathan Druart from comment #8)
> Could you please detail why you need this change?
> I will break the following use case:
> - Start to fill a form
> - *Ring belt end of the day*
> - you hurry up to get back at home quickly
> - Tomorrow morning, you finish to fill the form
> - Submit
> - You lost your changes
> 
> Ok it's a bit far-fetched but I don't understand what will bring us this
> 8-hours limitation.

You only need the token between loading the form and submitting it. I do not understand why you need 7 days for doing so?

Suppose an attacker got a CSRF token somehow from one user. Now he only needs that user to click on a malicious Koha URL that also sends the token. The amount of danger is obviously directly related to the length of the expiry period. Shorter is better, but should be balanced with ease of use.
Comment 10 Jonathan Druart 2016-08-24 08:59:20 UTC
Created attachment 54806 [details] [review]
Bug 17110: Lower CSRF expiry in Koha::Token

Default expiry in WWW:CSRF is one week.
This patch sets it to 8 hours by default in Koha, and allows to
change the expiry period individually by passing MaxAge.

Test plan:
[1] Put items in your cart.
[2] Apply the example patch too.
[3] Send the cart from opac within the allotted 10 seconds.
[4] Send again, but wait some 10 seconds before submitting. Too late!

Tested 3 patches together, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Jonathan Druart 2016-08-24 09:01:22 UTC
Created attachment 54807 [details] [review]
Bug 17110: Add unit test for MaxAge parameter in Token.t

Test plan:
Run t/Token.t

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Jonathan Druart 2016-08-24 09:24:49 UTC
I do not make up my mind on what it's best. So let it go and see later if we need to adapt it.
Another idea was to use max(pref(timeout), 8h or 24h) for the value of CSRF_EXPIRY_HOURS
Comment 13 Marcel de Rooy 2016-08-24 11:16:51 UTC
(In reply to Jonathan Druart from comment #12)
> I do not make up my mind on what it's best. So let it go and see later if we
> need to adapt it.
> Another idea was to use max(pref(timeout), 8h or 24h) for the value of
> CSRF_EXPIRY_HOURS

I will open up a new report for lowering the default timeout value.
If that meets approval, we could change this accordingly.
Thx for QA.

PS
Note that we also still have bug 15428..
And now bug 17187.
Comment 14 Kyle M Hall 2016-09-09 14:44:24 UTC
Pushed to master for 16.11, thanks Marcel!
Comment 15 Mason James 2017-05-03 01:08:59 UTC
(In reply to Kyle M Hall from comment #14)
> Pushed to master for 16.11, thanks Marcel!

Pushed to 16.05.x, for 16.05.12 release (required for BZ-17096)