From 9e74db7b51085f62919e34ab4e5ccdf9da2066a1 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: Mason James --- Koha/Token.pm | 50 ++++++++++++++++++++++++-------- basket/sendbasket.pl | 6 +++- members/member-flags.pl | 11 ++------ members/member-password.pl | 14 ++++++--- members/memberentry.pl | 17 +++++++++++ members/moremember.pl | 4 +++ opac/opac-memberentry.pl | 14 ++++----- opac/opac-sendbasket.pl | 7 ++++- t/Token.t | 58 ++++++++++++++++++++++++++++++++++---- tools/import_borrowers.pl | 14 ++++++++- tools/picture-upload.pl | 18 ++++++++++++ 11 files changed, 172 insertions(+), 41 deletions(-) diff --git a/Koha/Token.pm b/Koha/Token.pm index 9e26800596..89399bcd07 100644 --- a/Koha/Token.pm +++ b/Koha/Token.pm @@ -34,9 +34,10 @@ Koha::Token - Tokenizer type => 'CSRF', id => $id, secret => $secret, }); - # or check a CSRF token + # generate/check CSRF token with defaults and session id + my $csrf_token = $tokenizer->generate_csrf({ session_id => $x }); my $result = $tokenizer->check_csrf({ - id => $id, secret => $secret, token => $token, + session_id => $x, token => $token, }); =head1 DESCRIPTION @@ -51,6 +52,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; @@ -71,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. @@ -82,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 ); } @@ -91,19 +97,22 @@ sub generate { =head2 generate_csrf - Shortcut for: generate({ type => 'CSRF', ... }) + Like: generate({ type => 'CSRF', ... }) + Note: id defaults to userid from context, secret to database password. + session_id is mandatory; it is combined with id. =cut 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. @@ -121,17 +130,35 @@ sub check { =head2 check_csrf - Shortcut for: check({ type => 'CSRF', ... }) + Like: check({ type => 'CSRF', ... }) + Note: id defaults to userid from context, secret to database password. + session_id is mandatory; it is combined with id. =cut sub check_csrf { my ( $self, $params ) = @_; + return if !$params->{session_id}; + $params = _add_default_csrf_params( $params ); return $self->check({ %$params, type => 'CSRF' }); } # --- Internal routines --- +sub _add_default_csrf_params { + my ( $params ) = @_; + $params->{session_id} //= ''; + if( !$params->{id} ) { + $params->{id} = Encode::encode( 'UTF-8', C4::Context->userenv->{id} . $params->{session_id} ); + } else { + $params->{id} .= $params->{session_id}; + } + $params->{id} //= Encode::encode( 'UTF-8', C4::Context->userenv->{id} ); + my $pw = C4::Context->config('pass'); + $params->{secret} //= md5_base64( Encode::encode( 'UTF-8', $pw ) ), + return $params; +} + sub _gen_csrf { # Since WWW::CSRF::generate_csrf_token does not use the NonBlocking @@ -155,12 +182,13 @@ 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 14c988239a..6bcde5f99d 100755 --- a/basket/sendbasket.pl +++ b/basket/sendbasket.pl @@ -21,7 +21,6 @@ use warnings; use CGI qw ( -utf8 ); use Encode qw(encode); use Carp; - use Mail::Sendmail; use MIME::QuotedPrint; use MIME::Base64; @@ -50,6 +49,10 @@ my $email_add = $query->param('email_add'); my $dbh = C4::Context->dbh; if ( $email_add ) { + die "Wrong CSRF token" unless Koha::Token->new->check_csrf({ + session_id => scalar $query->cookie('CGISESSID'), + token => scalar $query->param('csrf_token'), + }); my $email = Koha::Email->new(); my %mail = $email->create_message_headers({ to => $email_add }); my $comment = $query->param('comment'); @@ -173,6 +176,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({ session_id => scalar $query->cookie('CGISESSID'), }), ); output_html_with_http_headers $query, $cookie, $template->output; } diff --git a/members/member-flags.pl b/members/member-flags.pl index ccddc395a2..e1e6bc0398 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; @@ -47,8 +45,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') ) ), + session_id => scalar $input->cookie('CGISESSID'), token => scalar $input->param('csrf_token'), }); @@ -208,11 +205,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( { session_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 85e4b976ec..b8035fdffa 100755 --- a/members/member-password.pl +++ b/members/member-password.pl @@ -18,8 +18,6 @@ use CGI qw ( -utf8 ); use C4::Members::Attributes qw(GetBorrowerAttributes); use Koha::Patron::Images; -use Digest::MD5 qw(md5_base64); - my $input = new CGI; my $theme = $input->param('theme') || "default"; @@ -63,8 +61,15 @@ my $minpw = C4::Context->preference('minPasswordLength'); push( @errors, 'SHORTPASSWORD' ) if ( $newpassword && $minpw && ( length($newpassword) < $minpw ) ); if ( $newpassword && !scalar(@errors) ) { - my $digest = Koha::AuthUtils::hash_password( $input->param('newpassword') ); - my $uid = $input->param('newuserid'); + + die "Wrong CSRF token" + unless Koha::Token->new->check_csrf({ + session_id => scalar $input->cookie('CGISESSID'), + token => scalar $input->param('csrf_token'), + }); + + my $digest = Koha::AuthUtils::hash_password( scalar $input->param('newpassword') ); + my $uid = $input->param('newuserid') || $bor->{userid}; my $dbh = C4::Context->dbh; if ( changepassword( $uid, $member, $digest ) ) { $template->param( newpassword => $newpassword ); @@ -141,6 +146,7 @@ $template->param( activeBorrowerRelationship => ( C4::Context->preference('borrowerRelationship') ne '' ), minPasswordLength => $minpw, RoutingSerials => C4::Context->preference('RoutingSerials'), + csrf_token => Koha::Token->new->generate_csrf({ session_id => scalar $input->cookie('CGISESSID'), }), ); if ( scalar(@errors) ) { diff --git a/members/memberentry.pl b/members/memberentry.pl index a34c260470..1675ebbfeb 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -282,6 +282,13 @@ if ( ( defined $newdata{'userid'} && $newdata{'userid'} eq '' ) || $check_Borrow $debug and warn join "\t", map {"$_: $newdata{$_}"} qw(dateofbirth dateenrolled dateexpiry); my $extended_patron_attributes = (); if ($op eq 'save' || $op eq 'insert'){ + + die "Wrong CSRF token" + unless Koha::Token->new->check_csrf({ + session_id => scalar $input->cookie('CGISESSID'), + token => scalar $input->param('csrf_token'), + }); + # If the cardnumber is blank, treat it as null. $newdata{'cardnumber'} = undef if $newdata{'cardnumber'} =~ /^\s*$/; @@ -683,6 +690,16 @@ $template->param( NoUpdateLogin => $NoUpdateLogin ); +# Generate CSRF token +$template->param( csrf_token => + Koha::Token->new->generate_csrf( { session_id => scalar $input->cookie('CGISESSID'), } ), +); + +# HouseboundModule data +$template->param( + housebound_role => Koha::Patron::HouseboundRoles->find($borrowernumber), +); + if(defined($data{'flags'})){ $template->param(flags=>$data{'flags'}); } diff --git a/members/moremember.pl b/members/moremember.pl index d5455c0e4d..ee5249bc71 100755 --- a/members/moremember.pl +++ b/members/moremember.pl @@ -274,6 +274,10 @@ if ( C4::Context->preference('NorwegianPatronDBEnable') && C4::Context->preferen # patronimage related interface on 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({ session_id => $input->cookie('CGISESSID'),}), +); my $branch=C4::Context->userenv->{'branch'}; diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl index 99affac04c..d1b66158e3 100755 --- a/opac/opac-memberentry.pl +++ b/opac/opac-memberentry.pl @@ -19,6 +19,8 @@ use Modern::Perl; use CGI qw ( -utf8 ); use Digest::MD5 qw( md5_base64 md5_hex ); +use JSON; +use List::MoreUtils qw( any each_array uniq ); use String::Random qw( random_string ); use HTML::Entities; @@ -184,8 +186,7 @@ elsif ( $action eq 'update' ) { my $borrower = GetMember( borrowernumber => $borrowernumber ); die "Wrong CSRF token" unless Koha::Token->new->check_csrf({ - id => $borrower->{userid}, - secret => md5_base64( C4::Context->config('pass') ), + session_id => scalar $cgi->cookie('CGISESSID'), token => scalar $cgi->param('csrf_token'), }); @@ -205,8 +206,7 @@ elsif ( $action eq 'update' ) { invalid_form_fields => $invalidformfields, borrower => \%borrower, csrf_token => Koha::Token->new->generate_csrf({ - id => $borrower->{userid}, - secret => md5_base64( C4::Context->config('pass') ), + session_id => scalar $cgi->cookie('CGISESSID'), }), ); @@ -240,8 +240,7 @@ elsif ( $action eq 'update' ) { nochanges => 1, borrower => GetMember( borrowernumber => $borrowernumber ), csrf_token => Koha::Token->new->generate_csrf({ - id => $borrower->{userid}, - secret => md5_base64( C4::Context->config('pass') ), + session_id => scalar $cgi->cookie('CGISESSID'), }), ); } @@ -263,8 +262,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 => $borrower->{userid}, - secret => md5_base64( C4::Context->config('pass') ), + session_id => scalar $cgi->cookie('CGISESSID'), }), ); diff --git a/opac/opac-sendbasket.pl b/opac/opac-sendbasket.pl index 97b64fd6d6..bf4f1a1c0a 100755 --- a/opac/opac-sendbasket.pl +++ b/opac/opac-sendbasket.pl @@ -23,7 +23,6 @@ use warnings; use CGI qw ( -utf8 ); use Encode qw(encode); use Carp; - use Mail::Sendmail; use MIME::QuotedPrint; use MIME::Base64; @@ -52,6 +51,10 @@ my $email_add = $query->param('email_add'); my $dbh = C4::Context->dbh; if ( $email_add ) { + die "Wrong CSRF token" unless Koha::Token->new->check_csrf({ + session_id => scalar $query->cookie('CGISESSID'), + token => scalar $query->param('csrf_token'), + }); my $email = Koha::Email->new(); my $user = GetMember(borrowernumber => $borrowernumber); my $user_email = GetFirstValidEmailAddress($borrowernumber) @@ -193,6 +196,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( + { session_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 dcc182e484..3a971a577d 100644 --- a/t/Token.t +++ b/t/Token.t @@ -20,26 +20,72 @@ # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. use Modern::Perl; -use Test::More tests => 6; +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, 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, 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 8c03274fcf..1ad886c2fa 100755 --- a/tools/import_borrowers.pl +++ b/tools/import_borrowers.pl @@ -56,7 +56,6 @@ use Text::CSV; # č use CGI qw ( -utf8 ); -# use encoding 'utf8'; # don't do this my (@errors, @feedback); my $extended = C4::Context->preference('ExtendedPatronAttributes'); @@ -109,6 +108,12 @@ my $overwrite_cardnumber = $input->param('overwrite_cardnumber'); $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({ + session_id => scalar $input->cookie('CGISESSID'), + token => scalar $input->param('csrf_token'), + }); + push @feedback, {feedback=>1, name=>'filename', value=>$uploadborrowers, filename=>$uploadborrowers}; my $handle = $input->upload('uploadborrowers'); my $uploadinfo = $input->uploadInfo($uploadborrowers); @@ -380,6 +385,13 @@ if ( $uploadborrowers && length($uploadborrowers) > 0 ) { } $template->param(matchpoints => \@matchpoints); } + + $template->param( + csrf_token => Koha::Token->new->generate_csrf( + { session_id => scalar $input->cookie('CGISESSID'), } + ), + ); + } output_html_with_http_headers $input, $cookie, $template->output; diff --git a/tools/picture-upload.pl b/tools/picture-upload.pl index 9892eb5d17..7d0dda3cdd 100755 --- a/tools/picture-upload.pl +++ b/tools/picture-upload.pl @@ -82,6 +82,13 @@ our %errors = (); # Case is important in these operational values as the template must use case to be visually pleasing! if ( ( $op eq 'Upload' ) && $uploadfile ) { + + die "Wrong CSRF token" + unless Koha::Token->new->check_csrf({ + session_id => scalar $input->cookie('CGISESSID'), + token => scalar $input->param('csrf_token'), + }); + my $dirname = File::Temp::tempdir( CLEANUP => 1 ); $debug and warn "dirname = $dirname"; my $filesuffix; @@ -163,6 +170,12 @@ elsif ( ( $op eq 'Upload' ) && !$uploadfile ) { $template->param( filetype => $filetype ); } elsif ( $op eq 'Delete' ) { + die "Wrong CSRF token" + unless Koha::Token->new->check_csrf({ + session_id => scalar $input->cookie('CGISESSID'), + token => scalar $input->param('csrf_token'), + }); + my $deleted = eval { Koha::Patron::Images->find( $borrowernumber )->delete; }; @@ -175,6 +188,11 @@ if ( $borrowernumber && !%errors && !$template->param('ERRORS') ) { "/cgi-bin/koha/members/moremember.pl?borrowernumber=$borrowernumber"); } else { + $template->param( + csrf_token => Koha::Token->new->generate_csrf({ + session_id => scalar $input->cookie('CGISESSID'), + }), + ); output_html_with_http_headers $input, $cookie, $template->output; } -- 2.39.5