From 7d964b674f90407870898d8cc6eff3eb9a004cfa 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 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 --- Koha/Token.pm | 20 ++++++++++++------- basket/sendbasket.pl | 10 ++-------- members/deletemem.pl | 13 +++--------- members/member-flags.pl | 11 ++--------- members/member-password.pl | 11 ++--------- members/memberentry.pl | 13 +++--------- members/moremember.pl | 7 +------ opac/opac-memberentry.pl | 13 ++++-------- opac/opac-sendbasket.pl | 11 +++-------- t/Token.t | 49 ++++++++++++++++++++++++++++++++++++++-------- tools/import_borrowers.pl | 9 ++------- tools/picture-upload.pl | 11 +++-------- 12 files changed, 79 insertions(+), 99 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/basket/sendbasket.pl b/basket/sendbasket.pl index ab29824..4a82edf 100755 --- a/basket/sendbasket.pl +++ b/basket/sendbasket.pl @@ -20,7 +20,6 @@ use Modern::Perl; use CGI qw ( -utf8 ); use Encode qw(encode); use Carp; -use Digest::MD5 qw(md5_base64); use Mail::Sendmail; use MIME::QuotedPrint; use MIME::Base64; @@ -52,8 +51,7 @@ my $dbh = C4::Context->dbh; if ( $email_add ) { die "Wrong CSRF token" unless Koha::Token->new->check_csrf({ - id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), + id => scalar $query->cookie('CGISESSID'), token => scalar $query->param('csrf_token'), }); my $email = Koha::Email->new(); @@ -176,11 +174,7 @@ else { url => "/cgi-bin/koha/basket/sendbasket.pl", suggestion => C4::Context->preference("suggestion"), virtualshelves => C4::Context->preference("virtualshelves"), - csrf_token => Koha::Token->new->generate_csrf( - { id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), - } - ), + csrf_token => Koha::Token->new->generate_csrf({ id => scalar $query->cookie('CGISESSID'), }), ); output_html_with_http_headers $query, $cookie, $template->output; } diff --git a/members/deletemem.pl b/members/deletemem.pl index 9ab49fb..47190bf 100755 --- a/members/deletemem.pl +++ b/members/deletemem.pl @@ -25,8 +25,6 @@ use strict; #use warnings; FIXME - Bug 2505 use CGI qw ( -utf8 ); -use Digest::MD5 qw(md5_base64); -use Encode qw( encode ); use C4::Context; use C4::Output; use C4::Auth; @@ -147,19 +145,14 @@ if ( $op eq 'delete_confirm' or $countissues > 0 or $flags->{'CHARGES'} or $is_ if ( not $countissues > 0 and not $flags->{CHARGES} ne '' and not $is_guarantor and not $deletelocal == 0 ) { $template->param( op => 'delete_confirm', - csrf_token => Koha::Token->new->generate_csrf( - { id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), - } - ), + csrf_token => Koha::Token->new->generate_csrf({ id => scalar $input->cookie('CGISESSID') }), ); } } elsif ( $op eq 'delete_confirmed' ) { die "Wrong CSRF token" - unless Koha::Token->new->check_csrf({ - id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), + unless Koha::Token->new->check_csrf( { + id => $input->cookie('CGISESSID'), token => scalar $input->param('csrf_token'), }); my $patron = Koha::Patrons->find( $member ); diff --git a/members/member-flags.pl b/members/member-flags.pl index d54984a..8509f62 100755 --- a/members/member-flags.pl +++ b/members/member-flags.pl @@ -8,8 +8,6 @@ use strict; use warnings; use CGI qw ( -utf8 ); -use Digest::MD5 qw(md5_base64); -use Encode qw( encode ); use C4::Output; use C4::Auth qw(:DEFAULT :EditPermissions); use C4::Context; @@ -48,8 +46,7 @@ if ($input->param('newflags')) { die "Wrong CSRF token" unless Koha::Token->new->check_csrf({ - id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), + id => scalar $input->cookie('CGISESSID'), token => scalar $input->param('csrf_token'), }); @@ -217,11 +214,7 @@ $template->param( is_child => ($bor->{'category_type'} eq 'C'), activeBorrowerRelationship => (C4::Context->preference('borrowerRelationship') ne ''), RoutingSerials => C4::Context->preference('RoutingSerials'), - csrf_token => Koha::Token->new->generate_csrf( - { id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), - } - ), + csrf_token => Koha::Token->new->generate_csrf( { id => scalar $input->cookie('CGISESSID'), } ), ); output_html_with_http_headers $input, $cookie, $template->output; diff --git a/members/member-password.pl b/members/member-password.pl index 1008cb6..6fa51c9 100755 --- a/members/member-password.pl +++ b/members/member-password.pl @@ -20,9 +20,6 @@ use Koha::Token; use Koha::Patron::Categories; -use Digest::MD5 qw(md5_base64); -use Encode qw( encode ); - my $input = new CGI; my $theme = $input->param('theme') || "default"; @@ -69,8 +66,7 @@ if ( $newpassword && !scalar(@errors) ) { die "Wrong CSRF token" unless Koha::Token->new->check_csrf({ - id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), + id => scalar $input->cookie('CGISESSID'), token => scalar $input->param('csrf_token'), }); @@ -150,10 +146,7 @@ $template->param( activeBorrowerRelationship => ( C4::Context->preference('borrowerRelationship') ne '' ), minPasswordLength => $minpw, RoutingSerials => C4::Context->preference('RoutingSerials'), - csrf_token => Koha::Token->new->generate_csrf({ - id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), - }), + csrf_token => Koha::Token->new->generate_csrf({ id => scalar $input->cookie('CGISESSID'), }), ); if ( scalar(@errors) ) { diff --git a/members/memberentry.pl b/members/memberentry.pl index ef0aa90..811f038 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -25,8 +25,6 @@ use warnings; # external modules use CGI qw ( -utf8 ); use List::MoreUtils qw/uniq/; -use Digest::MD5 qw(md5_base64); -use Encode qw( encode ); # internal modules use C4::Auth; @@ -290,8 +288,7 @@ if ($op eq 'save' || $op eq 'insert'){ die "Wrong CSRF token" unless Koha::Token->new->check_csrf({ - id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), + id => scalar $input->cookie('CGISESSID'), token => scalar $input->param('csrf_token'), }); @@ -750,12 +747,8 @@ $template->param( ); # Generate CSRF token -$template->param( - csrf_token => Koha::Token->new->generate_csrf( - { id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), - } - ), +$template->param( csrf_token => + Koha::Token->new->generate_csrf( { id => scalar $input->cookie('CGISESSID'), } ), ); # HouseboundModule data diff --git a/members/moremember.pl b/members/moremember.pl index 7330cb9..e8f160c 100755 --- a/members/moremember.pl +++ b/members/moremember.pl @@ -36,8 +36,6 @@ use strict; #use warnings; FIXME - Bug 2505 use CGI qw ( -utf8 ); -use Digest::MD5 qw(md5_base64); -use Encode qw( encode ); use C4::Context; use C4::Auth; use C4::Output; @@ -277,10 +275,7 @@ my $patron_image = Koha::Patron::Images->find($data->{borrowernumber}); $template->param( picture => 1 ) if $patron_image; # Generate CSRF token for upload and delete image buttons $template->param( - csrf_token => Koha::Token->new->generate_csrf({ - id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), - }), + csrf_token => Koha::Token->new->generate_csrf({id => $input->cookie('CGISESSID'),}), ); diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl index c1610a1..00d3ce2 100755 --- a/opac/opac-memberentry.pl +++ b/opac/opac-memberentry.pl @@ -19,7 +19,6 @@ use Modern::Perl; use CGI qw ( -utf8 ); use Digest::MD5 qw( md5_base64 md5_hex ); -use Encode qw( encode ); use String::Random qw( random_string ); use C4::Auth; @@ -200,8 +199,7 @@ elsif ( $action eq 'update' ) { my $borrower = GetMember( borrowernumber => $borrowernumber ); die "Wrong CSRF token" unless Koha::Token->new->check_csrf({ - id => Encode::encode( 'UTF-8', $borrower->{userid} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), + id => scalar $cgi->cookie('CGISESSID'), token => scalar $cgi->param('csrf_token'), }); @@ -221,8 +219,7 @@ elsif ( $action eq 'update' ) { invalid_form_fields => $invalidformfields, borrower => \%borrower, csrf_token => Koha::Token->new->generate_csrf({ - id => Encode::encode( 'UTF-8', $borrower->{userid} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), + id => scalar $cgi->cookie('CGISESSID'), }), ); @@ -262,8 +259,7 @@ elsif ( $action eq 'update' ) { nochanges => 1, borrower => GetMember( borrowernumber => $borrowernumber ), csrf_token => Koha::Token->new->generate_csrf({ - id => Encode::encode( 'UTF-8', $borrower->{userid} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), + id => scalar $cgi->cookie('CGISESSID'), }), ); } @@ -285,8 +281,7 @@ elsif ( $action eq 'edit' ) { #Display logged in borrower's data guarantor => scalar Koha::Patrons->find($borrowernumber)->guarantor(), hidden => GetHiddenFields( $mandatory, 'modification' ), csrf_token => Koha::Token->new->generate_csrf({ - id => Encode::encode( 'UTF-8', $borrower->{userid} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), + id => scalar $cgi->cookie('CGISESSID'), }), ); diff --git a/opac/opac-sendbasket.pl b/opac/opac-sendbasket.pl index c8c5e48..df2db5a 100755 --- a/opac/opac-sendbasket.pl +++ b/opac/opac-sendbasket.pl @@ -22,7 +22,6 @@ use Modern::Perl; use CGI qw ( -utf8 ); use Encode qw(encode); use Carp; -use Digest::MD5 qw(md5_base64); use Mail::Sendmail; use MIME::QuotedPrint; use MIME::Base64; @@ -54,8 +53,7 @@ my $dbh = C4::Context->dbh; if ( $email_add ) { die "Wrong CSRF token" unless Koha::Token->new->check_csrf({ - id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), + id => scalar $query->cookie('CGISESSID'), token => scalar $query->param('csrf_token'), }); my $email = Koha::Email->new(); @@ -196,11 +194,8 @@ else { url => "/cgi-bin/koha/opac-sendbasket.pl", suggestion => C4::Context->preference("suggestion"), virtualshelves => C4::Context->preference("virtualshelves"), - csrf_token => Koha::Token->new->generate_csrf( - { id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), - } - ), + csrf_token => Koha::Token->new->generate_csrf( + { id => scalar $query->cookie('CGISESSID'), } ), ); output_html_with_http_headers $query, $cookie, $template->output; } 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" ); +}; diff --git a/tools/import_borrowers.pl b/tools/import_borrowers.pl index daf7d59..d4bbcff 100755 --- a/tools/import_borrowers.pl +++ b/tools/import_borrowers.pl @@ -59,8 +59,6 @@ use Text::CSV; # č use CGI qw ( -utf8 ); -use Digest::MD5 qw(md5_base64); -use Encode qw( encode ); my (@errors, @feedback); my $extended = C4::Context->preference('ExtendedPatronAttributes'); @@ -112,8 +110,7 @@ $template->param( SCRIPT_NAME => '/cgi-bin/koha/tools/import_borrowers.pl' ); if ( $uploadborrowers && length($uploadborrowers) > 0 ) { die "Wrong CSRF token" unless Koha::Token->new->check_csrf({ - id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), + id => scalar $input->cookie('CGISESSID'), token => scalar $input->param('csrf_token'), }); @@ -391,9 +388,7 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { $template->param( csrf_token => Koha::Token->new->generate_csrf( - { id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), - } + { id => scalar $input->cookie('CGISESSID'), } ), ); diff --git a/tools/picture-upload.pl b/tools/picture-upload.pl index a3b549e..36f293d 100755 --- a/tools/picture-upload.pl +++ b/tools/picture-upload.pl @@ -25,8 +25,6 @@ use File::Temp; use File::Copy; use CGI qw ( -utf8 ); use GD; -use Digest::MD5 qw(md5_base64); -use Encode qw( encode ); use C4::Context; use C4::Auth; use C4::Output; @@ -88,8 +86,7 @@ if ( ( $op eq 'Upload' ) && $uploadfile ) { die "Wrong CSRF token" unless Koha::Token->new->check_csrf({ - id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), + id => scalar $input->cookie('CGISESSID'), token => scalar $input->param('csrf_token'), }); @@ -176,8 +173,7 @@ elsif ( ( $op eq 'Upload' ) && !$uploadfile ) { elsif ( $op eq 'Delete' ) { die "Wrong CSRF token" unless Koha::Token->new->check_csrf({ - id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), + id => scalar $input->cookie('CGISESSID'), token => scalar $input->param('csrf_token'), }); @@ -195,8 +191,7 @@ if ( $borrowernumber && !%errors && !$template->param('ERRORS') ) { else { $template->param( csrf_token => Koha::Token->new->generate_csrf({ - id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} ), - secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), + id => scalar $input->cookie('CGISESSID'), }), ); output_html_with_http_headers $input, $cookie, $template->output; -- 2.1.4