Bug 17110 - Lower CSRF expiry in Koha::Token
Summary: Lower CSRF expiry in Koha::Token
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 17109
Blocks: 17096
  Show dependency treegraph
 
Reported: 2016-08-11 13:25 UTC by Marcel de Rooy
Modified: 2017-05-03 01:08 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17110: Lower CSRF expiry in Koha::Token (1.25 KB, patch)
2016-08-11 13:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17110: Add unit test for MaxAge parameter in Token.t (1.34 KB, patch)
2016-08-12 06:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17110: [EXAMPLE PATCH] CSRF expiry of 10 seconds in opac-sendbasket (838 bytes, patch)
2016-08-12 06:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17110: Lower CSRF expiry in Koha::Token (1.38 KB, patch)
2016-08-15 13:25 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17110: Add unit test for MaxAge parameter in Token.t (1.42 KB, patch)
2016-08-15 13:26 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17110: [EXAMPLE PATCH] CSRF expiry of 10 seconds in opac-sendbasket (927 bytes, patch)
2016-08-15 13:27 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17110: Lower CSRF expiry in Koha::Token (1.45 KB, patch)
2016-08-24 08:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17110: Add unit test for MaxAge parameter in Token.t (1.50 KB, patch)
2016-08-24 09:01 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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)