From 3562816dd1b8855c7973ce5650ff834407c1a548 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 15 Feb 2017 17:14:13 +0100 Subject: [PATCH] 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 Signed-off-by: Julian Maurice Signed-off-by: Kyle M Hall --- 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 e60ee1f4fc..c560fce06f 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 0c56cd6d6e..3a971a577d 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.39.5