Bug 17830 - CSRF token is not generated correctly (bis)
Summary: CSRF token is not generated correctly (bis)
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low blocker (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 17096
Blocks:
  Show dependency treegraph
 
Reported: 2016-12-29 16:52 UTC by Jonathan Druart
Modified: 2017-01-24 23:59 UTC (History)
5 users (show)

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


Attachments
Bug 17830: CSRF - Handle unicode characters in userid (11.58 KB, patch)
2016-12-29 16:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF]-Bug-17830-CSRF-Handle-unicode-characters in userid (11.64 KB, patch)
2016-12-29 18:22 UTC, Karam Qubsi
Details | Diff | Splinter Review
Bug 17830: CSRF - Handle unicode characters in userid (11.68 KB, patch)
2016-12-30 17:49 UTC, Kyle M Hall
Details | Diff | Splinter Review

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