Currently the CSRF token generated is based on the borrowernumber, and is valid across user's session. We need to restrict the CSRF token to the current session.
Created attachment 60266 [details] [review] Bug 18124: Restrict CSRF token to user's session Currently the CSRF token generated is based on the borrowernumber, and is valid across user's session. We need to restrict the CSRF token to the current session. With this patch the CSRF token is generated concataining the id (borrowernumber) and the CGISESSID cookie. Test plan: Confirm that the different forms of the scripts modified by this patch still works correctly. Test the problematic behavior: Open 2 tabs with in same user's session, go on the edit patron page (memberentry.pl). Log out and log in from the other tab. Submit the form => Wrong CSRF token should be raised
Looking here now
lyyGength ? Adding a follow-up to rearrange parameters.
Created attachment 60326 [details] [review] Bug 18124: [Follow-up] Handle default parameters in a sub Adds a internal routine to handle default values for the parameters id and secret. Also adds a parameter session_id for generate_csrf and check_csrf. This session parameter is combined with the id parameter when generating or checking a token. Test plan: Run t/Token.t
Created attachment 60332 [details] [review] Bug 18124: Restrict CSRF token to user's session Currently the CSRF token generated is based on the borrowernumber, and is valid across user's session. We need to restrict the CSRF token to the current session. With this patch the CSRF token is generated concatenating the id (borrowernumber) and the CGISESSID cookie. Test plan: Run t/Token.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60333 [details] [review] Bug 18124: [Follow-up] Handle default parameters in a sub Adds a internal routine to handle default values for the parameters id and secret. Also adds a parameter session_id for generate_csrf and check_csrf. This session parameter is combined with the id parameter when generating or checking a token. Test plan: Run t/Token.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60334 [details] [review] Bug 18124: Change the calls to generate and check CSRF tokens The parameter change in Koha::Token should be applied to the calling scripts. Test plan: Confirm that the different forms of the scripts modified by this patch still work correctly. Test the problematic behavior: Open 2 tabs with in same user's session, go on the edit patron page (memberentry.pl). Log out and log in from the other tab. Submit the form => Wrong CSRF token should be raised Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 61282 [details] [review] Bug 18124: Restrict CSRF token to user's session Currently the CSRF token generated is based on the borrowernumber, and is valid across user's session. We need to restrict the CSRF token to the current session. With this patch the CSRF token is generated concatenating the id (borrowernumber) and the CGISESSID cookie. Test plan: Run t/Token.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 61283 [details] [review] Bug 18124: [Follow-up] Handle default parameters in a sub Adds a internal routine to handle default values for the parameters id and secret. Also adds a parameter session_id for generate_csrf and check_csrf. This session parameter is combined with the id parameter when generating or checking a token. Test plan: Run t/Token.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 61284 [details] [review] Bug 18124: Change the calls to generate and check CSRF tokens The parameter change in Koha::Token should be applied to the calling scripts. Test plan: Confirm that the different forms of the scripts modified by this patch still work correctly. Test the problematic behavior: Open 2 tabs with in same user's session, go on the edit patron page (memberentry.pl). Log out and log in from the other tab. Submit the form => Wrong CSRF token should be raised Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Passed QA
Created attachment 61688 [details] [review] Bug 18124: Change the calls to generate and check CSRF tokens The parameter change in Koha::Token should be applied to the calling scripts. Test plan: Confirm that the different forms of the scripts modified by this patch still work correctly. Test the problematic behavior: Open 2 tabs with in same user's session, go on the edit patron page (memberentry.pl). Log out and log in from the other tab. Submit the form => Wrong CSRF token should be raised Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Pushed to master for 17.05, thanks Jonathan, Marcel!
These patches have been pushed to 16.11.x and will be in 16.11.07. There was a small conflict in memberentry.pl in the dependencies list.
[04:13] <Joubu> mtj: around? [04:16] <Joubu> @later tell mtj 18124 is in 16.05.x and breaks members/moremember.pl [04:16] <huginn_> Joubu: The operation succeeded. mtj to follow this up...
(In reply to Mason James from comment #15) > [04:13] <Joubu> mtj: around? > [04:16] <Joubu> @later tell mtj 18124 is in 16.05.x and breaks > members/moremember.pl > [04:16] <huginn_> Joubu: The operation succeeded. > > mtj to follow this up... Hi Joubu, I've tested this patch on 16.05.x OK, and I see no regressions can you add a bit more info please?
Pushed to 16.05.x, for 16.05.12 release
(In reply to Mason James from comment #16) > (In reply to Mason James from comment #15) > > [04:13] <Joubu> mtj: around? > > [04:16] <Joubu> @later tell mtj 18124 is in 16.05.x and breaks > > members/moremember.pl > > [04:16] <huginn_> Joubu: The operation succeeded. > > > > mtj to follow this up... > > Hi Joubu, > I've tested this patch on 16.05.x OK, and I see no regressions > > can you add a bit more info please? Hit /members/moremember.pl?borrowernumber=42 or /members/memberentry.pl?borrowernumber=42 => Can't locate object method "new" via package "Koha::Token" (perhaps you forgot to load "Koha::Token"?) at /home/vagrant/kohaclone/members/memberentry.pl line 694.
It seems that you forgot to push bug 17146.
(In reply to Jonathan Druart from comment #19) > It seems that you forgot to push bug 17146. hi Jonathan, patch for BZ-17146 is now pushed to 16.05.x 8aa1e40 Bug 17146: Fix CSRF in picture-upload.pl