Bug 18124 - CSRF tokens must be limited to user's session
Summary: CSRF tokens must be limited to user's session
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 17096 18019 29812
  Show dependency treegraph
 
Reported: 2017-02-15 16:13 UTC by Jonathan Druart
Modified: 2022-01-07 10:29 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18124: Restrict CSRF token to user's session (21.69 KB, patch)
2017-02-15 16:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18124: [Follow-up] Handle default parameters in a sub (5.91 KB, patch)
2017-02-16 11:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18124: Restrict CSRF token to user's session (6.28 KB, patch)
2017-02-16 12:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18124: [Follow-up] Handle default parameters in a sub (7.47 KB, patch)
2017-02-16 12:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18124: Change the calls to generate and check CSRF tokens (16.13 KB, patch)
2017-02-16 12:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18124: Restrict CSRF token to user's session (6.31 KB, patch)
2017-03-20 11:44 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18124: [Follow-up] Handle default parameters in a sub (7.50 KB, patch)
2017-03-20 11:44 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18124: Change the calls to generate and check CSRF tokens (16.21 KB, patch)
2017-03-20 11:44 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18124: Change the calls to generate and check CSRF tokens (16.37 KB, patch)
2017-03-29 15:36 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 Jonathan Druart 2017-02-15 16:13:33 UTC
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.
Comment 1 Jonathan Druart 2017-02-15 16:19:50 UTC
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
Comment 2 Marcel de Rooy 2017-02-16 09:53:40 UTC
Looking here now
Comment 3 Marcel de Rooy 2017-02-16 11:03:33 UTC
lyyGength ?
Adding a follow-up to rearrange parameters.
Comment 4 Marcel de Rooy 2017-02-16 11:04:28 UTC
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
Comment 5 Marcel de Rooy 2017-02-16 12:35:14 UTC
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>
Comment 6 Marcel de Rooy 2017-02-16 12:35:19 UTC
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>
Comment 7 Marcel de Rooy 2017-02-16 12:35:24 UTC
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>
Comment 8 Julian Maurice 2017-03-20 11:44:07 UTC
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>
Comment 9 Julian Maurice 2017-03-20 11:44:11 UTC
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>
Comment 10 Julian Maurice 2017-03-20 11:44:15 UTC
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>
Comment 11 Julian Maurice 2017-03-20 11:45:38 UTC
Passed QA
Comment 12 Jonathan Druart 2017-03-29 15:36:36 UTC
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>
Comment 13 Kyle M Hall 2017-03-30 09:48:04 UTC
Pushed to master for 17.05, thanks Jonathan, Marcel!
Comment 14 Katrin Fischer 2017-04-02 16:05:13 UTC
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.
Comment 15 Mason James 2017-05-02 23:18:54 UTC
[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...
Comment 16 Mason James 2017-05-03 01:59:55 UTC
(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?
Comment 17 Mason James 2017-05-03 03:51:46 UTC
Pushed to 16.05.x, for 16.05.12 release
Comment 18 Jonathan Druart 2017-05-03 14:09:27 UTC
(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.
Comment 19 Jonathan Druart 2017-05-03 14:12:42 UTC
It seems that you forgot to push bug 17146.
Comment 20 Mason James 2017-05-08 02:41:21 UTC
(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