[Bug 17110] New: Lower CSRF expiry in Koha::Token
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 Bug ID: 17110 Summary: Lower CSRF expiry in Koha::Token Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: m.de.rooy@rijksmuseum.nl QA Contact: testopia@bugs.koha-community.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 --- Comment #1 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 54313 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54313&action=edit 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |m.de.rooy@rijksmuseum.nl --- Comment #2 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Example patch and adjusted test still coming.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |17109 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17109 [Bug 17109] sendbasket: Remove second authentication, add CSRF token -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |17096 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 54323 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54323&action=edit Bug 17110: Add unit test for MaxAge parameter in Token.t Test plan: Run t/Token.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 54324 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54324&action=edit Bug 17110: [EXAMPLE PATCH] CSRF expiry of 10 seconds in opac-sendbasket This patch should NOT be pushed ! It is used to test the first patch only. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #54313|0 |1 is obsolete| | --- Comment #5 from Marc Véron <veron@veron.ch> --- Created attachment 54473 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54473&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #54323|0 |1 is obsolete| | --- Comment #6 from Marc Véron <veron@veron.ch> --- Created attachment 54474 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54474&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #54324|0 |1 is obsolete| | --- Comment #7 from Marc Véron <veron@veron.ch> --- Created attachment 54475 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54475&action=edit Bug 17110: [EXAMPLE PATCH] CSRF expiry of 10 seconds in opac-sendbasket This patch should NOT be pushed ! It is used to test the first patch only. Signed-off-by: Marc Véron <veron@veron.ch> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |veron@veron.ch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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 are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #54473|0 |1 is obsolete| | Attachment #54474|0 |1 is obsolete| | Attachment #54475|0 |1 is obsolete| | --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 54806 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54806&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 --- Comment #11 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 54807 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54807&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 --- Comment #12 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=17187 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |kyle@bywatersolutions.com --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to master for 16.11, thanks Marcel! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtj@kohaaloha.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 --- Comment #15 from Mason James <mtj@kohaaloha.com> --- (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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #16 from David Cook <dcook@prosentient.com.au> --- I think time has shown that 8 hours is too short. Bug 42876 Bug 36586 - Self-checkouts will get CSRF errors if left inactive for 8 hours I think there's been other instances of CSRF tokens expiring before people can use them. Idle systems in general. I note on https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Pr... it says: "It's a common misconception to include timestamps as a value to specify the CSRF token expiration time. A CSRF Token is not an access token. They are used to verify the authenticity of requests throughout a session, using session information. A new session should generate a new token" Looking at https://metacpan.org/pod/WWW::CSRF#generate_csrf_token($id,-$secret,-\%options) We use the session ID for the ID (basically - so as to make it unique per user session), we use a server-side secret, and then the WWW::CSRF library forces us to use a timestamp which causes us the trouble. On https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Pr... it does say that per-request tokens are more secure than per-session tokens, but that it does impact usability, and that's something we've certainly encountered in Koha. If you look at Django and other frameworks, they typically do per-session tokens with per-request masking so as to prevent BREACH attacks. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=38797 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also|https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=38797 | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=42876 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17110 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42924 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42924 [Bug 42924] Restore default MaxAge for CSRF token -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org