Bug 17830

Summary: CSRF token is not generated correctly (bis)
Product: Koha Reporter: Jonathan Druart <jonathan.druart+koha>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart+koha>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: blocker    
Priority: P5 - low CC: karamqubsi, katrin.fischer, kyle, m.de.rooy, mtj
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17720
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18124
Change sponsored?: --- Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 17096    
Bug Blocks:    
Attachments: Bug 17830: CSRF - Handle unicode characters in userid
[SIGNED-OFF]-Bug-17830-CSRF-Handle-unicode-characters in userid
Bug 17830: CSRF - Handle unicode characters in userid

Description Jonathan Druart 2016-12-29 16:52:49 UTC
The same error as bug 17720 can occur if the userid has unicode characters.

Wide character in subroutine entry at /usr/share/perl5/Digest/HMAC.pm line 63.
Comment 1 Jonathan Druart 2016-12-29 16:58:27 UTC
Created attachment 58502 [details] [review]
Bug 17830: CSRF - Handle unicode characters in userid

If the userid of the logged in user contains unicode characters, the token
will not be generated correctly and Koha will crash with:
  Wide character in subroutine entry at /usr/share/perl5/Digest/HMAC.pm line 63.

Test plan:
- Edit a superlibrarian user and set his/her userid to '❤' or any other strings
with unicode characters.
- Login using this patron
- Search for patrons and click on a result.

=> Without this patch, you will get a software error (with "Wide
character in subroutine entry" in the logs).
=> With this patch, everything will go fine

You can also test the other files modified by this patch.
Comment 2 Karam Qubsi 2016-12-29 18:22:58 UTC
Created attachment 58515 [details] [review]
[SIGNED-OFF]-Bug-17830-CSRF-Handle-unicode-characters in userid


If the userid of the logged in user contains unicode characters, the token
will not be generated correctly and Koha will crash with:
  Wide character in subroutine entry at /usr/share/perl5/Digest/HMAC.pm line 63.

Test plan:
- Edit a superlibrarian user and set his/her userid to '❤' or any other strings
with unicode characters.
- Login using this patron
- Search for patrons and click on a result.

=> Without this patch, you will get a software error (with "Wide
character in subroutine entry" in the logs).
=> With this patch, everything will go fine

You can also test the other files modified by this patch.

Signed-off-by: Karam Qubsi <karamqubsi@gmail.com>

Patch is solving the problem as described .
Comment 3 Marcel de Rooy 2016-12-30 09:43:00 UTC
Why not move those encodes to sub _gen_csrf in Token.pm?
Since the source of this problem actually is WWW::CSRF, we could have this workaround closer to the call.
Comment 4 Jonathan Druart 2016-12-30 10:11:47 UTC
(In reply to Marcel de Rooy from comment #3)
> Why not move those encodes to sub _gen_csrf in Token.pm?
> Since the source of this problem actually is WWW::CSRF, we could have this
> workaround closer to the call.

I wanted to provide a quick and safe fix.
Moving the encode to Koha::Token would have implied to move the existing ones as well (i.e. more changes).
Comment 5 Kyle M Hall 2016-12-30 17:49:09 UTC
Created attachment 58535 [details] [review]
Bug 17830: CSRF - Handle unicode characters in userid

If the userid of the logged in user contains unicode characters, the token
will not be generated correctly and Koha will crash with:
  Wide character in subroutine entry at /usr/share/perl5/Digest/HMAC.pm line 63.

Test plan:
- Edit a superlibrarian user and set his/her userid to '❤' or any other strings
with unicode characters.
- Login using this patron
- Search for patrons and click on a result.

=> Without this patch, you will get a software error (with "Wide
character in subroutine entry" in the logs).
=> With this patch, everything will go fine

You can also test the other files modified by this patch.

Signed-off-by: Karam Qubsi <karamqubsi@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Kyle M Hall 2016-12-30 17:49:40 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 7 Katrin Fischer 2017-01-04 22:02:49 UTC
This patch has been pushed to 16.11.x and will be in 16.11.02.
Comment 8 Mason James 2017-05-03 02:52:59 UTC
patchset applied via BZ-18124 for 16.05.x, for 16.05.12 release