Bugzilla – Attachment 60332 Details for
Bug 18124
CSRF tokens must be limited to user's session
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 18124: Restrict CSRF token to user's session
Bug-18124-Restrict-CSRF-token-to-users-session.patch (text/plain), 6.28 KB, created by
Marcel de Rooy
on 2017-02-16 12:35:14 UTC
(
hide
)
Description:
Bug 18124: Restrict CSRF token to user's session
Filename:
MIME Type:
Creator:
Marcel de Rooy
Created:
2017-02-16 12:35:14 UTC
Size:
6.28 KB
patch
obsolete
>From 897a62cd6078eb637d0ccb6959c3e57469c9e855 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Wed, 15 Feb 2017 17:14:13 +0100 >Subject: [PATCH] Bug 18124: Restrict CSRF token to user's session >Content-Type: text/plain; charset=utf-8 > >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> >--- > Koha/Token.pm | 20 +++++++++++++------- > t/Token.t | 49 +++++++++++++++++++++++++++++++++++++++++-------- > 2 files changed, 54 insertions(+), 15 deletions(-) > >diff --git a/Koha/Token.pm b/Koha/Token.pm >index e60ee1f..c560fce 100644 >--- a/Koha/Token.pm >+++ b/Koha/Token.pm >@@ -51,6 +51,8 @@ use Modern::Perl; > use Bytes::Random::Secure (); > use String::Random (); > use WWW::CSRF (); >+use Digest::MD5 qw(md5_base64); >+use Encode qw( encode ); > use base qw(Class::Accessor); > use constant HMAC_SHA1_LENGTH => 20; > use constant CSRF_EXPIRY_HOURS => 8; # 8 hours instead of 7 days.. >@@ -72,7 +74,7 @@ sub new { > > my $token = $tokenizer->generate({ length => 20 }); > my $csrf_token = $tokenizer->generate({ >- type => 'CSRF', id => $id, secret => $secret, >+ type => 'CSRF', id => $id, > }); > > Generate several types of tokens. Now includes CSRF. >@@ -83,7 +85,10 @@ sub new { > sub generate { > my ( $self, $params ) = @_; > if( $params->{type} && $params->{type} eq 'CSRF' ) { >- $self->{lasttoken} = _gen_csrf( $params ); >+ $self->{lasttoken} = _gen_csrf( { >+ id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} . $params->{id} ), >+ secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), >+ }); > } else { > $self->{lasttoken} = _gen_rand( $params ); > } >@@ -98,13 +103,14 @@ sub generate { > > sub generate_csrf { > my ( $self, $params ) = @_; >+ return unless $params->{id}; > return $self->generate({ %$params, type => 'CSRF' }); > } > > =head2 check > > my $result = $tokenizer->check({ >- type => 'CSRF', id => $id, secret => $secret, token => $token, >+ type => 'CSRF', id => $id, token => $token, > }); > > Check several types of tokens. Now includes CSRF. >@@ -156,12 +162,12 @@ sub _gen_csrf { > > sub _chk_csrf { > my ( $params ) = @_; >- return if !$params->{id} || !$params->{secret} || !$params->{token}; >+ return if !$params->{id} || !$params->{token}; > >+ my $id = Encode::encode( 'UTF-8', C4::Context->userenv->{id} . $params->{id} ); >+ my $secret = md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ); > my $csrf_status = WWW::CSRF::check_csrf_token( >- $params->{id}, >- $params->{secret}, >- $params->{token}, >+ $id, $secret, $params->{token}, > { MaxAge => $params->{MaxAge} // ( CSRF_EXPIRY_HOURS * 3600 ) }, > ); > return $csrf_status == WWW::CSRF::CSRF_OK(); >diff --git a/t/Token.t b/t/Token.t >index 0c56cd6..3a971a5 100644 >--- a/t/Token.t >+++ b/t/Token.t >@@ -20,39 +20,72 @@ > # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. > > use Modern::Perl; >-use Test::More tests => 8; >+use Test::More tests => 10; > use Time::HiRes qw|usleep|; >+use C4::Context; > use Koha::Token; > >+C4::Context->_new_userenv('DUMMY SESSION'); >+C4::Context->set_userenv(0,42,0,'firstname','surname', 'CPL', 'Library 1', 0, ', '); >+ > my $tokenizer = Koha::Token->new; > is( length( $tokenizer->generate ), 1, "Generate without parameters" ); > my $token = $tokenizer->generate({ length => 20 }); > is( length($token), 20, "Token $token has 20 chars" ); > >-my $id = $tokenizer->generate({ length => 8 }); >-my $secr = $tokenizer->generate({ length => 32 }); >-my $csrftoken = $tokenizer->generate_csrf({ id => $id, secret => $secr }); >+my $id = $tokenizer->generate({ lyyGength => 8 }); >+my $csrftoken = $tokenizer->generate_csrf({ id => $id }); > isnt( length($csrftoken), 0, "Token $csrftoken should not be empty" ); > > is( $tokenizer->check, undef, "Check without any parameters" ); > my $result = $tokenizer->check_csrf({ >- id => $id, secret => $secr, token => $csrftoken, >+ id => $id, token => $csrftoken, > }); > is( $result, 1, "CSRF token verified" ); > > $result = $tokenizer->check({ >- type => 'CSRF', id => $id, secret => $secr, token => $token, >+ type => 'CSRF', id => $id, token => $token, > }); > isnt( $result, 1, "This token is no CSRF token" ); > > # Test MaxAge parameter > my $age = 1; # 1 second > $result = $tokenizer->check_csrf({ >- id => $id, secret => $secr, token => $csrftoken, MaxAge => $age, >+ id => $id, token => $csrftoken, MaxAge => $age, > }); > is( $result, 1, "CSRF token still valid within one second" ); > usleep $age * 1000000 * 2; # micro (millionth) seconds + 100% > $result = $tokenizer->check_csrf({ >- id => $id, secret => $secr, token => $csrftoken, MaxAge => $age, >+ id => $id, token => $csrftoken, MaxAge => $age, > }); > isnt( $result, 1, "CSRF token expired after one second" ); >+ >+subtest 'Same id (cookie CGISESSID) with an other logged in user' => sub { >+ plan tests => 2; >+ $csrftoken = $tokenizer->generate_csrf({ id => $id }); >+ $result = $tokenizer->check_csrf({ >+ id => $id, token => $csrftoken, >+ }); >+ is( $result, 1, "CSRF token verified" ); >+ C4::Context->set_userenv(0,43,0,'firstname','surname', 'CPL', 'Library 1', 0, ', '); >+ $result = $tokenizer->check_csrf({ >+ id => $id, token => $csrftoken, >+ }); >+ is( $result, '', "CSRF token is not verified if another logged in user is using the same id" ); >+}; >+ >+subtest 'Same logged in user with another session (cookie CGISESSID)' => sub { >+ plan tests => 2; >+ C4::Context->set_userenv(0,42,0,'firstname','surname', 'CPL', 'Library 1', 0, ', '); >+ $csrftoken = $tokenizer->generate_csrf({ id => $id }); >+ $result = $tokenizer->check_csrf({ >+ id => $id, token => $csrftoken, >+ }); >+ is( $result, 1, "CSRF token verified" ); >+ # Get another session id >+ $id = $tokenizer->generate({ lyyGength => 8 }); >+ $result = $tokenizer->check_csrf({ >+ id => $id, token => $csrftoken, >+ }); >+ is( $result, '', "CSRF token is not verified if another session is used" ); >+}; >-- >2.1.4
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 18124
:
60266
|
60326
|
60332
|
60333
|
60334
|
61282
|
61283
|
61284
|
61688